Re: CVS commit: src/crypto/dist/openssl/apps

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Tue, 14 Apr 2009 19:59:46 +0200
    From:        Joerg Sonnenberger <joerg%britannica.bec.de@localhost>
    Message-ID:  <20090414175946.GA12943%britannica.bec.de@localhost>

  | toupper like the rest of ctype.h is defined for EOF and for all values
  | of a unsigned char.

Yes.   Which is why its arg cannot be u_char as you asserted in the
previous message.    Which incidentally is the only point upon which
I was commenting in my message.

I'm not about to get into discussions about the adequacy of the ctype
macros/functions (and EOF) on mythical architectures (whether or not
any ever existed in the dim past) where sizeof(u_char) == sizeof(int),
such things are just a waste of anyone with anything real to do's time
and energy - leave that to the language idiots who keep butchering C so
it can work on those non-existent systems even if anyone was really
silly enough to care to make it.

I didn't (mean to) comment on the "This cast is simply wrong" part of
your message, as I hadn't loooked at the code - I probably should
have edited that but out of my quote (but it is much easier to just
delete whole lines...)

But since both you, and David, have commented on that issue, I took
a look, and I certainly would not agree with "simply wrong".  I might
agree with "simply useless", after all, ctyoe.h has

        int     toupper(int);

so for any call

                x = toupper(y);

'y' is going to be converted to an int from any other type anyway,
so changing it to be instead

                x = toupper((int)y);

is certainly useless, but cannot conceivably be "wrong" as it is changing
nothing at all.    The original code might have been wrong, but there
doesn't seem to be a PR about that, so if it is, I guess it doesn't bother
anyone (I haven't analysed it, I have looked at openssl briefly in the
past, and it made me violently ill - I have no desire to repeat the
experience...)

I'm also not about to enter into any flame wars about the sanity of
many of gcc's warnings, or their usefulness, or even whether it is
better to ignore the things, disable them, or pander to them - that
kind of discussion also isn't worth anyone's time.

  | Nevertheless, casting to int just to shut up a
  | warning from GCC is almost always the wrong approach here, unless it has
  | been explicitly checked before that isascii() is true and I can't find
  | immediate evidence for that.

isascii() used to be a requirement imposed upon toupper(), but no
longer, originally (I suspect) because someone just realised that
vast quantities of code didn't do it correctly, and the easiest fix
was just to alter the implementation, rather than attempt to
enforce that rule - so it was deleted years ago.

More recently of course, this also permits toupper() to work on
code sets that are not ASCII, like Greek (in theory anyway, not
on NetBSD, or not yet anyway), which ought to be a good thing, right?
(That is of course, just the 1 byte codesets, which still makes it
defective, but not as bad as being ASCII only.)

kre