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