Re: Proposal: add new API to stringinfo - Mailing list pgsql-hackers

Hi Gurjeet,

Thanks for looking into my patch.

> On Fri, Jan 3, 2025 at 8:54 PM Tatsuo Ishii <ishii@postgresql.org> wrote:
>>
>> Attached is the patch for this.
> 
> Hi Tatsuo,
> 
> I believe the newline endings in your patches are causing the patch application
> to fail. I experienced this with your initial patch, as well as with the latest
> patch. Converting it to unix-style line endings seems to solve the problem. I'm
> using the `patch` utility supplied with macos 13, in case that matters.

I am using emacs + mew (MUA) and it gives the attachment MIME Type as
"Text/X-Patch(us-ascii)".  I suspect the MIME type makes a trouble on
some MUAs. Sorry for it. Next time I will change the MIME type to
"Application/Octet-Stream" which is used in most patch posts.

> - * Initialize a StringInfoData struct (with previously undefined contents)
> - * to describe an empty string.
> + * Initialize a StringInfoData struct (with previously undefined contents).
> + * The initial memory allocation size is specified by 'initsize'.
> 
> In the above hunk, your patch removes the text 'to describe an empty string'. I
> don't think it needs to do that.

You are right. Will fix.

> +#define initStringInfo(str) \
> +    initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)
> 
> In the above hunk, the macro definition is split over 2 lines which seems
> unnecessary. It seems to fit on one line just fine, with 80-car limit, as below.
> 
> #define initStringInfo(str) initStringInfoExtended(str, STRINGINFO_DEFAULT_SIZE)

Initially I did so but according to the "Committing checklist"
https://wiki.postgresql.org/wiki/Committing_checklist

It recommends to not write lines longer than 78-char, rather than
80-char.

>    Verify that long lines are not better broken into several shorter lines:
> git diff origin/master | grep -E '^(\+|diff)' | sed 's/^+//' | expand -t4 | awk "length > 78 || /^diff/"

> +/*------------------------
> + * initStringInfoExtended
> + * Initialize a StringInfoData struct (with previously undefined contents)
> + * to describe an empty string.
> + * The data buffer is allocated with size 'initsize'.
> + */
> 
> In the above hunk, it would look better if the last sentence was either made
> part of the previous paragraph, or if there was a blank line between the two
> paragraphs. I'd prefer the former, like below.
> 
> /*------------------------
>  * initStringInfoExtended
>  * Initialize a StringInfoData struct (with previously undefined contents) to
>  * describe an empty string. The data buffer is allocated with size 'initsize'.
>  */

Ok, will fix.

> Personally, I'd prefer the parameter to be called simply 'size', instead of
> 'initsize'. The fact that the the function names begin with 'make' and 'init', I
> feel the 'init' in parameter name is extraneous. But since this is a matter of
> taste, no strong objections from me; the committer may choose to name it however
> they prefer.

I don't have strong preference neither but thinking about the fact
that those functions initially allocate the specified size of memory,
then they stretch memory twice if the previous size is not sufficient,
probably 'initsize' would give less confusion. If we use the parameter
name 'size', some users may think that the functions keep on
allocating memory by the size specified by 'size' parameter.

> Just out of curiousity, would it be okay to shorten the 'Extended' in the name
> to just 'Ext'. makeStringInfoExt() and initStringInfoExt() seem to be sufficent
> to convey the meaning, while helping to keep line lenghts short at call sites.

I think it's a good idea. I will change 'Extended' to 'Ext' in the
next patch.

> A slightly edited commit message, if the committer wishes to use it:

Thanks.

>     Previously StringInfo APIs allocated buffers with fixed allocation size of

maybe we want to have 'initial' in front of "allocation size"?

     Previously StringInfo APIs allocated buffers with fixed initial allocation size of

>     Added new StringInfo APIs to allow callers to specify buffer size
> 
>     Previously StringInfo APIs allocated buffers with fixed allocation size of
>     1024 bytes. This may be too large and inappropriate for some callers that
>     can do with smaller memory buffers. To fix this, introduce new APIs that
>     allow callers to specify initial buffer size.
> 
>     extern StringInfo makeStringInfoExtended(int initsize);
>     extern void initStringInfoExtended(StringInfo str, int initsize);
> 
>     Existing APIs (makeStringInfo() and initStringInfo()) are now macros to call
>     makeStringInfoExtended() and initStringInfoExtended(), respectively, with
>     the default buffer size of 1024.

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp