Re: [PATCH] clarify palloc comment on quote_literal_cstr

On Mon, Apr 7, 2025 at 3:19 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Sun, Apr 06, 2025 at 12:37:24PM -0500, Steve Chavez wrote:
> > I found the numbers in `quote_literal_cstr` palloc quite magical. So I've
> > added a comment clarifying what they mean. The change is small:
> >
> > /* We make a worst-case result area; wasting a little space is OK */
> > - result = palloc(len * 2 + 3 + 1);
> > + result = palloc(
> > + (len * 2) /* worst-case doubling for every character if
> > each one is a quote */
> > + + 3 /* two outer quotes + possibly 'E' if needed */
> > + + 1 /* null terminator */
> > + );
>
> Agreed that this is a good idea to have a better documentation here if
> someone decides to tweak this area in the future, rather than have
> them guess the numbers.
>
> Looking at what quote_literal_internal() does, it seems to me that you
> are right based on ESCAPE_STRING_SYNTAX, SQL_STR_DOUBLE(), and the
> extra backslashes added to the destination buffer.

This is an improvement in readability. However, I have not seen this
style of commenting arguments of a function. If there's a precedence,
please let me know. Usually we explain it in a comment before the
call. For example, something like
/* Allocate memory for worst case result factoring in a. double the
length number of bytes, in case every character is a quote, b. two
bytes for two outer quotes c. extra byte for E' d. one byte for null
terminator. */

--
Best Wishes,
Ashutosh Bapat