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