Re: Proposal: add new API to stringinfo

> Hi Gurjeet,
>
> Thanks for looking into my patch.
>
>> On Fri, Jan 3, 2025 at 8:54 PM Tatsuo Ishii <ishii(at)postgresql(dot)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.

Attached is the v2 patch to reflect above.

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