Hi Calvin and Jonathan
On 26/05/2023 22:19, Jonathan Tan wrote:
Calvin Wan <calvinwan@xxxxxxxxxx> 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
Signed-off-by: Calvin Wan <calvinwan@xxxxxxxxxx>
---
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.
-/* 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.
-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.
