Re: [libvirt] [PATCH] avoid one more ctype vs. sign-extension problem
Daniel Veillard <veillard(a)redhat.com> wrote:
On Thu, May 08, 2008 at 04:44:38PM +0200, Jim Meyering wrote:
> This change demonstrates that the new syntax-check rule's
> regexp can be improved. It missed the unsafe tolower use,
> since there was already a to_uchar use on that line.
Since this shows up again, let me argue a bit more why I dislike
using is* and to* in libvirt.
I agree completely, especially since I've just fixed three
similar bugs in coreutils:
http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/13512
http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=209850fd7
I'm in the process of removing all of them from libvirt, since there
aren't many. Actually, I've just realized that the 'right' way to do
this is to change each use of isspace to c_isspace, isdigit to c_isdigit,
etc., relying on the gnulib c-ctype module:
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/c-ctype.h
Looking into that, I see more room for improvement, i.e., where things
like isdigit and isalnum have been open-coded. With these c_is* functions,
we can have portability, readability, *and* efficiency:
For example, compare these:
while ((*cur >= '0') && (*cur <= '9')) {
while (c_isdigit(*cur)) {
================================
and these:
char_ok =
(model[i] >= '0' && model[i] <= '9') ||
(model[i] >= 'a' && model[i] <= 'z') ||
(model[i] >= 'A' && model[i] <= 'Z') ||
model[i] == '_';
char_ok = c_isalnum (model[i]) || model[i] == '_';
So I'm making changes like these, too.