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....@gmail.com> wrote: > On Mon, Apr 7, 2025 at 3:19 AM Michael Paquier <mich...@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 >
From 99f64dfc687273ca1ef90199b8c6a9addb6578a9 Mon Sep 17 00:00:00 2001 From: steve-chavez <stevechavez...@gmail.com> Date: Mon, 7 Apr 2025 13:34:55 -0500 Subject: [PATCH] clarify palloc comment on quote_literal_cstr --- src/backend/utils/adt/quote.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/adt/quote.c b/src/backend/utils/adt/quote.c index 677efb93e8d..60bda20606d 100644 --- a/src/backend/utils/adt/quote.c +++ b/src/backend/utils/adt/quote.c @@ -107,7 +107,10 @@ quote_literal_cstr(const char *rawstr) int newlen; len = strlen(rawstr); - /* We make a worst-case result area; wasting a little space is OK */ + /* 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. */ result = palloc(len * 2 + 3 + 1); newlen = quote_literal_internal(result, rawstr, len); -- 2.40.1
