Horton

Re: formatting.c cleanup

Peter Eisentraut <peter@eisentraut.org>

View thread View on postgr.es
From: Peter Eisentraut <peter@eisentraut.org>
To: Chao Li <li.evan.chao@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-10-31T09:18:05Z
Lists: pgsql-hackers 
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 more or less mechanical passes over it to modernize the code a bit, renamed some symbols to be clearer, adjusted some types to make things more self-documenting, removed some redundant code to make some things more compact.  I hope this helps a 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, exceeding 80 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 structure is 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’ because maxlen 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.

Commits

  1. formatting.c cleanup: Change fill_str() return type to void

  2. formatting.c cleanup: Change TmFromChar.clock field to bool

  3. formatting.c cleanup: Change several int fields to enums

  4. formatting.c cleanup: Rename DCH_S_* to DCH_SUFFIX_*

  5. formatting.c cleanup: Improve formatting of some struct declarations

  6. formatting.c cleanup: Remove unnecessary zeroize macros

  7. formatting.c cleanup: Remove unnecessary extra line breaks in error message literals

  8. formatting.c cleanup: Remove unnecessary extra parentheses

  9. formatting.c cleanup: Use array syntax instead of pointer arithmetic

  10. formatting.c cleanup: Add some const pointer qualifiers

  11. formatting.c cleanup: Use size_t for string length variables and arguments

  12. formatting.c cleanup: Move loop variables definitions into for statement

  13. formatting.c cleanup: Remove dashes in comments