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.
