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

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.

```
$ patch -p 1 < v1-0001-Add-new-StringInfo-APIs.patch
patching file 'src/common/stringinfo.c'
1 out of 1 hunks failed--saving rejects to 'src/common/stringinfo.c.rej'
patching file 'src/include/lib/stringinfo.h'
3 out of 3 hunks failed--saving rejects to 'src/include/lib/stringinfo.h.rej'

$ dos2unix v1-0001-Add-new-StringInfo-APIs.patch
dos2unix: converting file v1-0001-Add-new-StringInfo-APIs.patch to
Unix format...

$ patch -p1 < v1-0001-Add-new-StringInfo-APIs.patch
patching file 'src/common/stringinfo.c'
patching file 'src/include/lib/stringinfo.h'
```

The GNU patch cli (from Homebrew) seems to be able to deal with CRLF line
terminators somewhat gracefully.

```
$ patch -p1 < v1-0001-Add-new-StringInfo-APIs.patch
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/common/stringinfo.c
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/include/lib/stringinfo.h
```

After dealing with all this, I noticed that your patch has the commit message,
as well. So tried to apply it with `git am` command, and that was clean, as
expected.

- * 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.

+#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)

+/*------------------------
+ * 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'.
 */

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.

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.

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

    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 regards,
Gurjeet
http://Gurje.et