[cfe-commits] r138995 - /cfe/trunk/lib/Sema/SemaExpr.cpp

[Richard Trieu]

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.