[cfe-commits] PATCH: Warn on left shifts which overflow, even when not due to the RHS

[Chandler Carruth]

This patch (originally by Oleg Slezberg) implements a warning that would
have caught several major bugs we found, so I'm pretty excited about it. The
fundamental premise is that the user writes code as follows:

int64_t i = 10 << 30;

The user thinks this is a nice 10 gig constant for their disk size. The
compiler thinks this is -2 gigs. Oops.

As it turns out, this is technically undefined behavior, and so it seems
doubly good to warn about it when we see it. The warning has a special case
though. It still provides a warning for the expression (1 << 31) because
it's undefined behavior to do this, but that warning is by default ignored.
If the value is converted to an unsigned integer, it will have the intended
value as all the bits did in fact get set. It seems much less important to
warn about this case. The result looks something like:

% ./bin/clang++ -Wall -Wshift-sign-overflow t.cc
t.cc:1:13: warning: shift result (10737418240) requires 35 bits to
represent, but the promoted type of the shift expression is 'int' with only
32 bits [-Wshift-overflow]
long l = 10 << 30;
         ~~ ^  ~~
t.cc:2:16: warning: shift result (2147483648) overrides the sign bit of the
promoted type of the shift expression ('int') and become negative
[-Wshift-sign-overflow]
unsigned u = 1 << 31;
             ~ ^  ~~
2 warnings generated.

One open question is whether we should issue any fixit hints. My original
theory was to issue a fixit hint on the warning which suggests adding an
'LL' suffix for the first case, and a 'U' suffix for the second case.
However, Chris was concerned about this fixit hint because if the user
writes:

f(10 << 30);

Perhaps they meant to write '10LL' or '1'. Correcting the code to '10LL'
would potentially change the overload selected, or have other strange and
unanticipated consequences. In our experience, the bugs we're seeing are all
fixed by the 'LL' suffix (potentially also fixing any mismatched result
type), but that's just anecdotal. Should this be a warning fixit hint (and
be automatically applied) or should it be a note fixit hint (and not)?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110220/635badfb/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: warn-shift-overflow.patch
Type: text/x-patch
Size: 17073 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110220/635badfb/attachment.bin>