Re: [PATCH v2 1/2] grep: use static trans-case table

From: Jeff King <peff@peff.net>
Date: Feb 29, 2012, 08:28 UTC
Message-ID: <20120229082814.GB14181@sigill.intra.peff.net>
In-Reply-To: <1330474831-9030-2-git-send-email-gitster@pobox.com>

On Tue, Feb 28, 2012 at 04:20:30PM -0800, Junio C Hamano wrote:

Show 5 quoted lines

> In order to prepare the kwset machinery for a case-insensitive search, we
> used to use a static table of 256 elements and filled it every time before
> calling kwsalloc().  Because the kwset machinery will never modify this
> table, just allocate a single instance globally and fill it at the compile
> time.

Hmm. I was going to complain that the original code used tolower() to generate the table at run-time, and therefore respected the current locale. But of course we have replaced tolower() with a locale-independent version, so it should behave identically.

But that does make me wonder. Do people expect their case-insensitive searches to work on non-ASCII characters? I would think yes, but I do not use non-ASCII characters in the first place, so my opinion may not mean much.

For that matter, does REG_ICASE respect locales? The glibc code appears to consider it, but I couldn't make it work in some simple tests. But if it does, that raises another weirdness: we fall back to kwset transparently when a grep pattern contains no metacharacters. So you would get different results for "-i --grep=é" versus "-i --grep=é.*".

Of course, even if we used a locale-respecting version of tolower in the original code, I suspect that a byte table would be fundamentally insufficient, anyway, in the face of multi-byte encodings like utf8.

So I don't think your patch is making the problem any worse. And even if somebody wants to tackle the problem later, the solution would look so unlike the original code that your change is not hurting their effort.

-Peff