<div class="gmail_quote">On Thu, Sep 1, 2011 at 8:48 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Author: rtrieu<br>
Date: Thu Sep 1 22:48:46 2011<br>
New Revision: 138995<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=138995&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=138995&view=rev</a><br>
Log:<br>
Move the warning for different enum comparisons and the warning for using NULL as a non-pointer in a binary operation into separate functions.<br>
<br>
Modified:<br>
cfe/trunk/lib/Sema/SemaExpr.cpp<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=138995&r1=138994&r2=138995&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=138995&r1=138994&r2=138995&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Sep 1 22:48:46 2011<br>
@@ -6167,6 +6167,35 @@<br>
return false;<br>
}<br>
<br>
+/// If two different enums are compared, raise a warning.<br>
+static void checkEnumComparison(Sema &S, SourceLocation Loc, ExprResult &lex,<br>
+ ExprResult &rex) {<br>
+ QualType lType = lex.get()->getType();<br>
+ QualType rType = rex.get()->getType();<br>
+ QualType LHSStrippedType = lex.get()->IgnoreParenImpCasts()->getType();<br>
+ QualType RHSStrippedType = rex.get()->IgnoreParenImpCasts()->getType();<br>
+<br>
+ const EnumType *LHSEnumType = LHSStrippedType->getAs<EnumType>();<br>
+ if (!LHSEnumType)<br>
+ return;<br>
+ const EnumType *RHSEnumType = RHSStrippedType->getAs<EnumType>();<br>
+ if (!RHSEnumType)<br>
+ return;<br>
+<br>
+ // Ignore anonymous enums.<br>
+ if (!LHSEnumType->getDecl()->getIdentifier())<br>
+ return;<br>
+ if (!RHSEnumType->getDecl()->getIdentifier())<br>
+ return;<br></blockquote><div><br></div><div>Why not just one if and return?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+<br>
+ if (S.Context.hasSameUnqualifiedType(LHSStrippedType, RHSStrippedType))<br>
+ return;<br>
+<br>
+ S.Diag(Loc, diag::warn_comparison_of_mixed_enum_types)<br>
+ << LHSStrippedType << RHSStrippedType<br>
+ << lex.get()->getSourceRange() << rex.get()->getSourceRange();<br>
+}<br>
+<br>
/// \brief Diagnose bad pointer comparisons.<br>
static void diagnoseDistinctPointerComparison(Sema &S, SourceLocation Loc,<br>
ExprResult &lex, ExprResult &rex,<br>
@@ -6254,20 +6283,7 @@<br>
QualType LHSStrippedType = LHSStripped->getType();<br>
QualType RHSStrippedType = RHSStripped->getType();<br>
<br>
-<br>
-<br>
- // Two different enums will raise a warning when compared.<br>
- if (const EnumType *LHSEnumType = LHSStrippedType->getAs<EnumType>()) {<br>
- if (const EnumType *RHSEnumType = RHSStrippedType->getAs<EnumType>()) {<br>
- if (LHSEnumType->getDecl()->getIdentifier() &&<br>
- RHSEnumType->getDecl()->getIdentifier() &&<br>
- !Context.hasSameUnqualifiedType(LHSStrippedType, RHSStrippedType)) {<br>
- Diag(Loc, diag::warn_comparison_of_mixed_enum_types)<br>
- << LHSStrippedType << RHSStrippedType<br>
- << lex.get()->getSourceRange() << rex.get()->getSourceRange();<br>
- }<br>
- }<br>
- }<br>
+ checkEnumComparison(*this, Loc, lex, rex);<br>
<br>
if (!lType->hasFloatingRepresentation() &&<br>
!(lType->isBlockPointerType() && isRelational) &&<br>
@@ -7576,6 +7592,54 @@<br>
<< lhs->getSourceRange() << rhs->getSourceRange();<br>
}<br>
<br>
+// checkArithmeticNull - Detect when a NULL constant is used improperly in an<br>
+// expression. These are mainly cases where the null pointer is used as an<br>
+// integer instead of a pointer.<br>
+static void checkArithmeticNull(Sema &S, ExprResult &lex, ExprResult &rex,<br>
+ SourceLocation Loc, bool isCompare) {<br>
+ // The canonical way to check for a GNU null is with isNullPointerConstant,<br>
+ // but we use a bit of a hack here for speed; this is a relatively<br>
+ // hot path, and isNullPointerConstant is slow.<br>
+ bool LeftNull = isa<GNUNullExpr>(lex.get()->IgnoreParenImpCasts());<br>
+ bool RightNull = isa<GNUNullExpr>(rex.get()->IgnoreParenImpCasts());<br>
+<br>
+ // Detect when a NULL constant is used improperly in an expression. These<br>
+ // are mainly cases where the null pointer is used as an integer instead<br>
+ // of a pointer.<br>
+ if (!LeftNull && !RightNull)<br>
+ return;<br>
+<br>
+ QualType LeftType = lex.get()->getType();<br>
+ QualType RightType = rex.get()->getType();<br></blockquote><div><br></div><div>I think it'd be good to instead compute the NonNullType here. That would save us a lot of work in the comparisons below.</div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+<br>
+ // Avoid analyzing cases where the result will either be invalid (and<br>
+ // diagnosed as such) or entirely valid and not something to warn about.<br>
+ if (LeftType->isBlockPointerType() || LeftType->isMemberPointerType() ||<br>
+ LeftType->isFunctionType() || RightType->isBlockPointerType() ||<br>
+ RightType->isMemberPointerType() || RightType->isFunctionType())<br>
+ return;<br></blockquote><div><br></div><div>For example we could check ((LeftNull && RightNull) || NonNullType->isFoo() || ...)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+<br>
+ // Comparison operations would not make sense with a null pointer no matter<br>
+ // what the other expression is.<br>
+ if (!isCompare) {<br>
+ S.Diag(Loc, diag::warn_null_in_arithmetic_operation)<br>
+ << (LeftNull ? lex.get()->getSourceRange() : SourceRange())<br>
+ << (RightNull ? rex.get()->getSourceRange() : SourceRange());<br>
+ return;<br>
+ }<br>
+<br>
+ // The rest of the operations only make sense with a null pointer<br>
+ // if the other expression is a pointer.<br>
+ if (LeftNull == RightNull || LeftType->isAnyPointerType() ||<br>
+ LeftType->canDecayToPointerType() || RightType->isAnyPointerType() ||<br>
+ RightType->canDecayToPointerType())<br>
+ return;<br></blockquote><div><br></div><div>Here again I think NonNullType would help. Also, I do find (LeftNull && RightNull) a bit more clear...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+<br>
+ S.Diag(Loc, diag::warn_null_in_comparison_operation)<br>
+ << LeftNull /* LHS is NULL */<br>
+ << (LeftNull ? rex.get()->getType() : lex.get()->getType())<br></blockquote><div><br></div><div>LeftType and RightType? (although NonNullType fires again here...)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+ << lex.get()->getSourceRange() << rex.get()->getSourceRange();<br>
+}<br>
/// CreateBuiltinBinOp - Creates a new built-in binary operation with<br>
/// operator @p Opc at location @c TokLoc. This routine only supports<br>
/// built-in operations; ActOnBinOp handles overloaded operators.<br>
@@ -7607,53 +7671,16 @@<br>
rhs = move(resolvedRHS);<br>
}<br>
<br>
- // The canonical way to check for a GNU null is with isNullPointerConstant,<br>
- // but we use a bit of a hack here for speed; this is a relatively<br>
- // hot path, and isNullPointerConstant is slow.<br>
- bool LeftNull = isa<GNUNullExpr>(lhs.get()->IgnoreParenImpCasts());<br>
- bool RightNull = isa<GNUNullExpr>(rhs.get()->IgnoreParenImpCasts());<br>
-<br>
- // Detect when a NULL constant is used improperly in an expression. These<br>
- // are mainly cases where the null pointer is used as an integer instead<br>
- // of a pointer.<br>
- if (LeftNull || RightNull) {<br>
- // Avoid analyzing cases where the result will either be invalid (and<br>
- // diagnosed as such) or entirely valid and not something to warn about.<br>
- QualType LeftType = lhs.get()->getType();<br>
- QualType RightType = rhs.get()->getType();<br>
- if (!LeftType->isBlockPointerType() && !LeftType->isMemberPointerType() &&<br>
- !LeftType->isFunctionType() &&<br>
- !RightType->isBlockPointerType() &&<br>
- !RightType->isMemberPointerType() &&<br>
- !RightType->isFunctionType()) {<br>
- if (Opc == BO_Mul || Opc == BO_Div || Opc == BO_Rem || Opc == BO_Add ||<br>
- Opc == BO_Sub || Opc == BO_Shl || Opc == BO_Shr || Opc == BO_And ||<br>
- Opc == BO_Xor || Opc == BO_Or || Opc == BO_MulAssign ||<br>
- Opc == BO_DivAssign || Opc == BO_AddAssign || Opc == BO_SubAssign ||<br>
- Opc == BO_RemAssign || Opc == BO_ShlAssign || Opc == BO_ShrAssign ||<br>
- Opc == BO_AndAssign || Opc == BO_OrAssign || Opc == BO_XorAssign) {<br>
- // These are the operations that would not make sense with a null pointer<br>
- // pointer no matter what the other expression is.<br>
- Diag(OpLoc, diag::warn_null_in_arithmetic_operation)<br>
- << (LeftNull ? lhs.get()->getSourceRange() : SourceRange())<br>
- << (RightNull ? rhs.get()->getSourceRange() : SourceRange());<br>
- } else if (Opc == BO_LE || Opc == BO_LT || Opc == BO_GE || Opc == BO_GT ||<br>
- Opc == BO_EQ || Opc == BO_NE) {<br>
- // These are the operations that would not make sense with a null pointer<br>
- // if the other expression the other expression is not a pointer.<br>
- if (LeftNull != RightNull &&<br>
- !LeftType->isAnyPointerType() &&<br>
- !LeftType->canDecayToPointerType() &&<br>
- !RightType->isAnyPointerType() &&<br>
- !RightType->canDecayToPointerType()) {<br>
- Diag(OpLoc, diag::warn_null_in_comparison_operation)<br>
- << LeftNull /* LHS is NULL */<br>
- << (LeftNull ? rhs.get()->getType() : lhs.get()->getType())<br>
- << lhs.get()->getSourceRange() << rhs.get()->getSourceRange();<br>
- }<br>
- }<br>
- }<br>
- }<br>
+ if (Opc == BO_Mul || Opc == BO_Div || Opc == BO_Rem || Opc == BO_Add ||<br>
+ Opc == BO_Sub || Opc == BO_Shl || Opc == BO_Shr || Opc == BO_And ||<br>
+ Opc == BO_Xor || Opc == BO_Or || Opc == BO_MulAssign ||<br>
+ Opc == BO_DivAssign || Opc == BO_AddAssign || Opc == BO_SubAssign ||<br>
+ Opc == BO_RemAssign || Opc == BO_ShlAssign || Opc == BO_ShrAssign ||<br>
+ Opc == BO_AndAssign || Opc == BO_OrAssign || Opc == BO_XorAssign)<br>
+ checkArithmeticNull(*this, lhs, rhs, OpLoc, /*isCompare=*/false);<br>
+ else if (Opc == BO_LE || Opc == BO_LT || Opc == BO_GE || Opc == BO_GT ||<br>
+ Opc == BO_EQ || Opc == BO_NE)<br>
+ checkArithmeticNull(*this, lhs, rhs, OpLoc, /*isCompare=*/true);<br></blockquote><div><br></div><div>Now that these are a simple function call, we can do my original goal in this *entire* refactoring: sink these calls down into the switch! That way we don't need these massive || chains.</div>
<div><br></div><div><br></div><div>Also, by the way, fantastic refactorings. Thanks for all the cleanup work here!!!</div></div>