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. > 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
