[cfe-commits] [Patch] Fix crash when attempting to dump a StringLiteral with wchar's in it.

[Richard Smith]

On Tue, Jun 12, 2012 at 11:14 PM, James Dennett <jdennett at google.com> wrote:

> On Tue, Jun 12, 2012 at 11:00 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > LGTM
> >
> > +      if (Char <= 0xff && isprint(Char))
> > +        OS << (char)Char;
> >
> > I appreciate this is just copy-pasted, but I noticed that the 'Char <=
> 0xff'
> > condition is always true here (and the 'else' case can't cope with cases
> > where Char > 0xff either). Maybe remove the 'Char <= 0xff' test?
>
> The range check before calling isprint is a good thing.
>
> If that's the standard isprint, it's undefined behavior to call it
> unless the argument has a value that either is EOF or fits into
> unsigned char.  Depending on the library implementation, isprint could
> realistically return bogus values for larger values, or could
> conceivably segfault.
>

Sure, but this code is unreachable in the case where Char > 0xff. The
'else' branch also doesn't work for Char > 0xff. The presence of the test
is simply misleading; it should either be an assert or should be removed.


> I'd sooner see a test against UCHAR_MAX than 0xff, but more as
> documentation than for correctness -- I don't expect Clang to run on a
> machine where CHAR_BIT != 8 (but I've been wrong before).
>

Yes, if we keep the test as an assert, switching to UCHAR_MAX could make
the intent clearer. UCHAR_MAX is guaranteed to be at least 0xff, so there's
no correctness issue.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120612/d4a0cde8/attachment.html>