[cfe-commits] [Patch] Fix crash when attempting to dump a StringLiteral with wchar's in it.
jdennett at google.com
Wed Jun 13 00:01:43 PDT 2012
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>
>> > 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.
More information about the cfe-commits