> Hi Gurjeet, > > Thanks for looking into my patch. > >> On Fri, Jan 3, 2025 at 8:54 PM Tatsuo Ishii <is...@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.
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
v2-0001-Add-new-StringInfo-APIs-to-allow-callers-to-speci.patch
Description: Binary data
