Re: [PATCH] clarify palloc comment on quote_literal_cstr

I haven't found a similar style of comment on any other function call.

I've attached a new patch using the style you suggest.

That being said, I do find the first form much more readable, but I
understand this is a debatable subject.

Best regards,
Steve Chavez

On Mon, 7 Apr 2025 at 06:33, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

> On Mon, Apr 7, 2025 at 3:19 AM Michael Paquier <michael@paquier.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
>