<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt">On Mon, Nov 12, 2012 at 6:35 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 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>> wrote:<br>
>> > Make -Wtautological-constant-out-of-range-compare checking take into<br>
>> > account types and conversion between types. The old version merely checked<br>
>> > the bit widths, which allowed failed to catch a few cases, while 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 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 will<br>
> be considered out of range when comparing against enums. I factored out the<br>
> width into a variable. The current code will remove the existing warnings<br>
> in warn-enum-compare.cpp. The code in the comments would have preserved<br>
> them.<br>
<br>
</div></div>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></blockquote><div>IntRange doesn't appear to take those special cases into consideration. From looking at the code, it appears to special case enum, otherwise it falls back to ASTContext::getIntWdith().</div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Again, if you want to change our enum-related warnings, please do it<br>
in a separate patch.<br>
<br></blockquote><div>Will do. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I would like to see more tests for the various cases in this code.<br>
Please make sure there's a test covering every branch you introduced.<br></blockquote><div><br></div><div>Also looking into it. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-Eli<br>
</font></span></blockquote></div><br></div>