On Tue, Jun 12, 2012 at 11:00 PM, Richard Smith <[email protected]> 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. 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). -- James _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
