René Scharfe [off-list ref] writes:
quoted
In the previous submission, I just moved isblank() and isgraph() as
implemented in wildmatch.c. I knew they were not robust against the
pointer increment like isblank(*s++), but I thought it was the same
pattern as isprint(), which has the same issue. Unfortunately, it was
more controversial than I had expected...
Not sure we need that story in the commit message,...
Usually we encourage people to write as if there were no "previous
iterations". Describe how to reach the best end-result without any
detour, using the experience gained from failed attempts.
... but it gave me an idea:
;-)
... So it's not so
easy, however, ...
quoted
This version implements them as inline functions because we ran out
all bits in the sane_ctype[] table. This is the same pattern as
islower() and isupper().
... if you remove GIT_SPACE from the definition above you get a
macro version of isgraph() that uses a single table lookup.
If we're out of bits then isblank() is a good choice to implement
without a table lookup, as this class only contains two characters
and two comparisons should be quite fast.
Implementing sane_isblank() plus sane_isgraph() as static inlines is
already not too bad, but if one of them can be made into a simple
table look-up, that indeed is better ;-)
Stepping back a bit: Is using the unlocalized is* macros in
wildmatch() safe, i.e. do we get the same results as before
regardless of locale? Junio's remark in
https://lore.kernel.org/git/xmqq3579crsd.fsf@gitster.g/ (local) sounds
convincing to me if we don't care about single-byte code pages
and require plain ASCII or UTF-8. I think it's a good idea to
address that point in the commit message.
True. To be honest, I wasn't thinking about non-UTF-8 single-byte
"code pages". In the context of wildmatch, the strings we deal with
mostly interact with the paths on the filesystem. If you interact
with your filesystem in latin-1 that may pose an issue, and even if
(or rather, especially if) we are to declare that it does not matter,
we should document it in the proposed log message.
And adding tests to t/helper/test-ctype.c would be nice.
Very true.
quoted
+#define isblank(x) sane_isblank(x)
+#define isgraph(x) sane_isgraph(x)
@@ -1270,6 +1274,16 @@ static inline int sane_iscase(int x, int is_lower)
return (x & 0x20) == 0;
}
+static inline int sane_isblank(int c)
+{
+ return c == ' ' || c == '\t';
+}
+
+static inline int sane_isgraph(int c)
+{
+ return isprint(c) && !isspace(c);
+}