[cfe-commits] r138995 - /cfe/trunk/lib/Sema/SemaExpr.cpp
Chandler Carruth
chandlerc at google.com
Fri Sep 2 00:36:36 PDT 2011
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!!!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110902/e83547f1/attachment.html>
More information about the cfe-commits
mailing list