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

Douglas Gregor dgregor at apple.com
Mon Feb 21 10:46:46 PST 2011


On Feb 20, 2011, at 8:21 PM, Chandler Carruth wrote:

> 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)?

Since the compiler is not implicitly adding the 'LL' suffix as part of recovery, this Fix-It should go on a note.

	- Doug

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110221/83b65a6b/attachment.html>


More information about the cfe-commits mailing list