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

[James Dennett]

On Tue, Jun 12, 2012 at 11:52 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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.

You're right.  Apologies, I didn't look at the context and see that
there's already a "break;" preceding this if Char is > 0xff.

An assertion would be nice documentation here, but not vital.