Re: [PATCH v2 3/7] sane-ctype.h: move sane-ctype macros from git-compat-util.h

From: Phillip Wood <phillip.wood123@gmail.com>
Date: May 28, 2023, 12:04 UTC
Message-ID: <4d29e5d4-6ce3-c099-15a7-e89ad787780d@gmail.com>
In-Reply-To: <20230526211911.2291886-1-jonathantanmy@google.com>

Hi Calvin and Jonathan

On 26/05/2023 22:19, Jonathan Tan wrote:

> Calvin Wan <calvinwan@google.com> writes:
>> Splitting these macros from git-compat-util.h cleans up the file and
>> allows future third-party sources to not use these overrides if they do
>> not wish to.

I think splitting these out makes sense. If you have time to add a comment highlighting the differences from the standard library versions that would be really helpful. As far as I know the differences are

  - our versions are locale independent
  - our isspace() does not consider '\v' or '\f' to be space
  - our versions expect a signed or unsigned char rather than an int
    as their argument
  - our versions do not accept EOF
  - our versions do not require an explicit cast to avoid undefined
    behavior when passing char on systems where char is signed

Show 7 quoted lines

>> Signed-off-by: Calvin Wan <calvinwan@google.com>
>> ---
>>   git-compat-util.h | 65 +-------------------------------------------
>>   sane-ctype.h      | 69 +++++++++++++++++++++++++++++++++++++++++++++++
> 
> Any specific reason for the "sane-" prefix? I think it would make more
> sense if it was just named ctype.h: see below.

I don't have a strong opinion either way but "sane-ctype.h" makes it clearer why we're doing something different to the standard library.

Show 6 quoted lines

>> -/* in ctype.c, for kwset users */
>> -extern const unsigned char tolower_trans_tbl[256];
> 
> I'm not sure what this has to do with sanity, but this is indeed defined
> in ctype.c, so it's easier to justify moving this out if the criterion
> was "what's in ctype.c" rather than "what's related to sane ctypes".

There is only one user of this so it could be moved to diffcore-pickaxe.c or kwset.c where it would be more discoverable if anyone needs to add another user in the future.

Show 5 quoted lines

>> -extern const signed char hexval_table[256];
> 
> And this one has nothing to do with ctypes or sanity, but rather, what's
> considered to be a hex character. I think we need another patch to move
> this to hex.h.

Isn't "what's considered to be a hex character" related to ctypes and sanity though as it is used by the "sane" definition of isxdigit().

>> -#define isxdigit(x) (hexval_table[(unsigned char)(x)] != -1)
> 
> Same for this one.

I'd much rather keep all of our "sane" ctype replacements in one place as Calvin is proposing. isxdigit() is defined in <ctype.h> so it should be in our "sane" version of that header.

Best Wishes

Phillip

> With the above suggestions, I think we do get what we want - a split
> between things that make ctype more sane and between other compat stuff,
> and also a split between those two and more platform-agnostic things
> like what a hex character is.