On 20.10.25 15:31, Chao Li wrote:
>> On Oct 20, 2025, at 17:50, Peter Eisentraut <peter@eisentraut.org> wrote:
>> The file formatting.c contains some hard to read and understand code. For the attached patch series, I made a few
moreor less mechanical passes over it to modernize the code a bit, renamed some symbols to be clearer, adjusted some
typesto make things more self-documenting, removed some redundant code to make some things more compact. I hope this
helpsa bit.
I have committed this patch set. Thanks for checking.
> 0004 - Changes “char *” to “const char *” wherever possible.
>
> I found some “text *” can be “const text *”:
I added those.
> 0007 - I am not convinced with this change. Combining two constant string into one causes the line too long,
exceeding80 columns.
We have been moving toward not breaking up string literals, except where
they contain a line break. This makes it easier to grep for error
messages, for example.
> 0010 - Changes TmFromChar.clock from int to bool.
>
> A suggestion is that maybe we can move the new field “clock_12_hour” next to “bool has_tz”, so that size of the
structureis reduced by 4 bytes.
I don't think we need to worry about structure size here. (If we did,
we could change many fields to short int, probably.) The current order
seems pretty logical. So I kept it.
> 0013 - Changes fill_str() to return void.
>
> ```
> -static char *
> +static void
> fill_str(char *str, int c, size_t maxlen)
> {
> memset(str, c, maxlen);
> str[maxlen] = '\0';
> - return str;
> }
> ```
>
> This function has no comment, so I am not sure what “maxlen” exactly mean. Usually, we do “str[maxlen-1] = ‘\0’
becausemaxlen usually implies max length of the buffer. But this function seems to fill maxlen of c into str, then
“maxlen”might not be a good name, “count” could be better.
Yes, this is a bit confusing. I added a function comment to explain this.
