<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt">On Tue, Nov 13, 2012 at 1:50 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>></span> wrote:<br>
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Nov 13, 2012 at 1:35 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
> On Mon, Nov 12, 2012 at 6:35 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Nov 12, 2012 at 5:31 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>> > I uploaded a new patch.<br>
>> ><br>
>> > On Mon, Nov 12, 2012 at 11:22 AM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Comments below.<br>
>> >><br>
>> >> On Mon, Nov 12, 2012 at 9:46 AM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>><br>
>> >> wrote:<br>
>> >> > Make -Wtautological-constant-out-of-range-compare checking take into<br>
>> >> > account types and conversion between types. The old version merely<br>
>> >> > checked<br>
>> >> > the bit widths, which allowed failed to catch a few cases, while<br>
>> >> > warning on<br>
>> >> > other safe comparisons.<br>
>> >> ><br>
>> >> > <a href="http://llvm-reviews.chandlerc.com/D113" target="_blank">http://llvm-reviews.chandlerc.com/D113</a><br>
>> >> ><br>
>> >> > Files:<br>
>> >> > test/Analysis/additive-folding.cpp<br>
>> >> > test/SemaCXX/compare.cpp<br>
>> >> > test/SemaCXX/warn-enum-compare.cpp<br>
>> >> > lib/Sema/SemaChecking.cpp<br>
>> >><br>
>> >> Index: test/SemaCXX/warn-enum-compare.cpp<br>
>> >> ===================================================================<br>
>> >> --- test/SemaCXX/warn-enum-compare.cpp<br>
>> >> +++ test/SemaCXX/warn-enum-compare.cpp<br>
>> >> @@ -39,8 +39,8 @@<br>
>> >> while (b == c);<br>
>> >> while (B1 == name1::B2);<br>
>> >> while (B2 == name2::B1);<br>
>> >> - while (x == AnonAA); // expected-warning {{comparison of constant<br>
>> >> 42 with expression of type 'Foo' is always false}}<br>
>> >> - while (AnonBB == y); // expected-warning {{comparison of constant<br>
>> >> 45 with expression of type 'Bar' is always false}}<br>
>> >> + while (x == AnonAA);<br>
>> >> + while (AnonBB == y);<br>
>> >> while (AnonAA == AnonAB);<br>
>> >> while (AnonAB == AnonBA);<br>
>> >> while (AnonBB == AnonAA);<br>
>> >><br>
>> >> Why are you changing this warning?<br>
>> ><br>
>> > See discussion below at IntRange.<br>
>> >><br>
>> >><br>
>> >> while (AnonBB == AnonAA);<br>
>> >> Index: lib/Sema/SemaChecking.cpp<br>
>> >> ===================================================================<br>
>> >> --- lib/Sema/SemaChecking.cpp<br>
>> >> +++ lib/Sema/SemaChecking.cpp<br>
>> >> @@ -4328,38 +4328,91 @@<br>
>> >> Expr *Constant, Expr *Other,<br>
>> >> llvm::APSInt Value,<br>
>> >> bool RhsConstant) {<br>
>> >> + // 0 values are handled later by CheckTrivialUnsignedComparison().<br>
>> >> + if (Value == 0)<br>
>> >> + return;<br>
>> >> +<br>
>> >> BinaryOperatorKind op = E->getOpcode();<br>
>> >> QualType OtherT = Other->getType();<br>
>> >> QualType ConstantT = Constant->getType();<br>
>> >> + QualType CommonT = E->getLHS()->getType();<br>
>> >> if (S.Context.hasSameUnqualifiedType(OtherT, ConstantT))<br>
>> >> return;<br>
>> >> assert((OtherT->isIntegerType() && ConstantT->isIntegerType())<br>
>> >> && "comparison with non-integer type");<br>
>> >> // FIXME. handle cases for signedness to catch (signed char)N == 200<br>
>> >><br>
>> >> Does this patch fix this FIXME?<br>
>> ><br>
>> > Yes. FIXME removed, added test case.<br>
>> >><br>
>> >><br>
>> >> - IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);<br>
>> >> - IntRange LitRange = GetValueRange(S.Context, Value,<br>
>> >> Value.getBitWidth());<br>
>> >> - if (OtherRange.Width >= LitRange.Width)<br>
>> >> +<br>
>> >> + bool ConstantSigned = ConstantT->isSignedIntegerType();<br>
>> >> + bool OtherSigned = OtherT->isSignedIntegerType();<br>
>> >> + bool CommonSigned = CommonT->isSignedIntegerType();<br>
>> >><br>
>> >> Why are you getting rid of the IntRange usage? Granted, we didn't<br>
>> >> really have any good testcases, but it does add power to the analysis<br>
>> >> in some cases.<br>
>> ><br>
>> > For my use, IntRange didn't add any value. Everything I needed, I<br>
>> > could<br>
>> > just get from the APSInt. The only difference was what width I used for<br>
>> > Other. IntRange does truncate the enum width so that different values<br>
>> > will<br>
>> > be considered out of range when comparing against enums. I factored out<br>
>> > the<br>
>> > width into a variable. The current code will remove the existing<br>
>> > warnings<br>
>> > in warn-enum-compare.cpp. The code in the comments would have preserved<br>
>> > them.<br>
>><br>
>> It does add value in cases other than enums; it can recognize<br>
>> bitfields and knows some tricks with binary "&", logical operators in<br>
>> C, etc.<br>
>><br>
> IntRange doesn't appear to take those special cases into consideration.<br>
> From looking at the code, it appears to special case enum, otherwise it<br>
> falls back to ASTContext::getIntWdith().<br>
<br>
</div></div>Oh! I was assuming the code was using the GetExprRange() function,<br>
but it looks like it isn't for reasons I don't really understand.<br>
<span class="HOEnZb"><font color="#888888"><br>
-Eli<br>
</font></span></blockquote></div><br><div>GetExprRange() looks promising for a tighter bound on the bit range. I will leave a FIXME to investigate it further.</div></div>