<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt">I uploaded a new patch.<br><br><div class="gmail_quote">On Mon, Nov 12, 2012 at 11:22 AM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Comments below.<br>
<div><div class="h5"><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 account types and conversion between types. The old version merely checked the bit widths, which allowed failed to catch a few cases, while warning on 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>
</div></div>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></blockquote><div>See discussion below at IntRange. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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>
<div class="im"> && "comparison with non-integer type");<br>
// FIXME. handle cases for signedness to catch (signed char)N == 200<br>
<br>
</div>Does this patch fix this FIXME?<br></blockquote><div>Yes. FIXME removed, added test case. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);<br>
- IntRange LitRange = GetValueRange(S.Context, Value, 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></blockquote><div> For my use, IntRange didn't add any value. Everything I needed, I could just get from the APSInt. The only difference was what width I used for Other. IntRange does truncate the enum width so that different values will be considered out of range when comparing against enums. I factored out the width into a variable. The current code will remove the existing warnings in warn-enum-compare.cpp. The code in the comments would have preserved them.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ bool EqualityOnly = false;<br>
+<br>
+ if (CommonSigned) {<br>
+ // The common type is signed, therefore no signed to unsigned conversion.<br>
+ if (OtherSigned) {<br>
+ // Check that the constant is representable in type OtherT.<br>
+ if (S.Context.getIntWidth(OtherT) >= Value.getMinSignedBits())<br>
<br>
Calling getMinSignedBits() on an unsigned constant will do the wrong<br>
thing in cases like 0xFFFFFFFFU. (I'm not sure it's actually possible<br>
to write a testcase where this matters, but it would make more sense<br>
anyway.)<br></blockquote><div>Added checks for signedness before using functions that depend on it. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ return;<br>
+ } else { // !OtherSigned<br>
+ // Check that the constant is representable in type OtherT.<br>
+ // Negative values are out of range.<br>
+ if (Value.isNonNegative() &&<br>
+ S.Context.getIntWidth(OtherT) >= Value.getActiveBits())<br>
<br>
Calling isNonNegative() on an unsigned constant can also do the wrong thing.<br></blockquote><div>Same here. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
.+ return;<br>
+ }<br>
+ } else { // !CommonSigned<br>
+ if (!OtherSigned && !ConstantSigned) {<br>
+ // Both types are unsigned, check the constant is representable in<br>
+ // type OtherT.<br>
<div class="im">+ if (S.Context.getIntWidth(OtherT) >= Value.getActiveBits())<br>
+ return;<br>
+ } else if (!OtherSigned && ConstantSigned) {<br>
+ if (S.Context.getIntWidth(OtherT) >= Value.getActiveBits())<br>
+ return;<br>
<br>
</div>Calling getActiveBits() on a signed value is suspicious; the return<br>
will vary with the width of the constant for the same value.<br></blockquote><div>And here. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ } else if (OtherSigned && !ConstantSigned) {<br>
+ // Check to see if the constant is representable in OtherT.<br>
+ if (S.Context.getIntWidth(OtherT) > Value.getActiveBits())<br>
+ return;<br>
+ // Check to see if the constant is equivalent to a negative value<br>
+ // cast to CommonT.<br>
+ if (S.Context.getIntWidth(ConstantT) == S.Context.getIntWidth(CommonT) &&<br>
+ Value.isNegative() &&<br>
<br>
Is this going to handle cases like (int)x == 0xFFFFFFFFFFFFFFFFULL correctly?<br></blockquote><div> Yes, it doesn't warn on that case.</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>