From: Thomas Martitz <thomas.martitz_at_student.htw-berlin.de>
Date: Wed, 12 Aug 2009 01:13:39 +0200

Al Le wrote:
> Hello.
>
> IMO the r22153 introduced a weirdly looking code:
>
> static inline int
> nat_toupper(int a)
> {
> return tolower(a);
> }
>
> This makes the question arise (to a new code reader) as to why
> "tolower" is called inside the function when the function itself is
> named "nat_toupper"
>
> Besides, I don't quite understand why this change should fix the
> problem mentioned in the commit message ("improper sorting of names
> with underscores when Interpret numbers when sorting is used"). Should
> the old code work correctly if toupper is implemented correctly? If
> the commit really fixes something, shouldn't the real cause be fixed
> (something in toupper IMO)?
>
> Just some thoughts by a casual code reader.

If you look at the ASCII-table, the underscore (and a few more chars) is
between the upper and lower case chars. And for case-insensitive
sorting, all chars are simply converted into upper or lower case.

Using toupper causes (for example) files starting with an _ to sort
behind normal files (with tolower they sort before). This, though, is
different from str(n|case)cmp which uses tolower. Hence strnatcmp sorted
files starting with an _ different from ASCII-sort. That wasn't the
intention when this sorting was committed and raised a bug report.
tolower/toupper and str(n|case)cmp shouldn't be changed as they're
standard functions.

I didn't change the name of nat_toupper because I would need to change
all references and thus change the code more from the orignal
implementation. Also, it's a wrapper for exactly this sort of thing,
allowing to change how it works without changing the code calling it. It
looks weird, indeed. But it shouldn't have been called nat_toupper in
the first place imo but something more generic instead.
Received on 2009-08-12