From: Phillip Wood Date: Sun, 28 May 2023 12:04:11 GMT Subject: Re: [PATCH v2 3/7] sane-ctype.h: move sane-ctype macros from git-compat-util.h 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 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 >> --- >> 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 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.