Re: [libvirt] [PATCH] avoid one more ctype vs. sign-extension problem

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Daniel Veillard <veillard@xxxxxxxxxx> 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.

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]