From abbb8ada4536f88a36a01d1afb084452c269ea5a Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Sun, 7 Jan 2018 21:57:48 +0000 Subject: [PATCH] Factor out common tautological comparison code from scalar and vector compare checking. In passing, improve vector compare diagnostic to match scalar compare diagnostic. llvm-svn: 321972 --- .../clang/Basic/DiagnosticSemaKinds.td | 2 +- clang/include/clang/Sema/Sema.h | 3 +- clang/lib/Sema/SemaExpr.cpp | 207 ++++++++++-------- clang/test/Sema/ext_vector_comparisons.c | 12 +- 4 files changed, 119 insertions(+), 105 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index d23f476d860f..8bf88cc8568f 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -7906,7 +7906,7 @@ def note_ref_subobject_of_member_declared_here : Note< // should result in a warning, since these always evaluate to a constant. // Array comparisons have similar warnings def warn_comparison_always : Warning< - "%select{self-|array }0comparison always evaluates to %select{false|true|a constant}1">, + "%select{self-|array }0comparison always evaluates to %select{a constant|%2}1">, InGroup; def warn_comparison_bitwise_always : Warning< "bitwise comparison always evaluates to %select{false|true}0">, diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 055d14f2d0de..ddc539ce3d92 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -9580,7 +9580,8 @@ public: bool AllowBothBool, bool AllowBoolConversion); QualType GetSignedVectorType(QualType V); QualType CheckVectorCompareOperands(ExprResult &LHS, ExprResult &RHS, - SourceLocation Loc, bool isRelational); + SourceLocation Loc, + BinaryOperatorKind Opc); QualType CheckVectorLogicalOperands(ExprResult &LHS, ExprResult &RHS, SourceLocation Loc); diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 7e01b058ab2d..933289360b28 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -9587,19 +9587,119 @@ static void diagnoseLogicalNotOnLHSofCheck(Sema &S, ExprResult &LHS, // Get the decl for a simple expression: a reference to a variable, // an implicit C++ field reference, or an implicit ObjC ivar reference. static ValueDecl *getCompareDecl(Expr *E) { - if (DeclRefExpr* DR = dyn_cast(E)) + if (DeclRefExpr *DR = dyn_cast(E)) return DR->getDecl(); - if (ObjCIvarRefExpr* Ivar = dyn_cast(E)) { + if (ObjCIvarRefExpr *Ivar = dyn_cast(E)) { if (Ivar->isFreeIvar()) return Ivar->getDecl(); } - if (MemberExpr* Mem = dyn_cast(E)) { + if (MemberExpr *Mem = dyn_cast(E)) { if (Mem->isImplicitAccess()) return Mem->getMemberDecl(); } return nullptr; } +/// Diagnose some forms of syntactically-obvious tautological comparison. +static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc, + Expr *LHS, Expr *RHS, + BinaryOperatorKind Opc) { + Expr *LHSStripped = LHS->IgnoreParenImpCasts(); + Expr *RHSStripped = RHS->IgnoreParenImpCasts(); + + QualType LHSType = LHS->getType(); + QualType RHSType = RHS->getType(); + if (LHSType->hasFloatingRepresentation() || + (LHSType->isBlockPointerType() && !BinaryOperator::isEqualityOp(Opc)) || + LHS->getLocStart().isMacroID() || RHS->getLocStart().isMacroID() || + S.inTemplateInstantiation()) + return; + + // For non-floating point types, check for self-comparisons of the form + // x == x, x != x, x < x, etc. These always evaluate to a constant, and + // often indicate logic errors in the program. + // + // NOTE: Don't warn about comparison expressions resulting from macro + // expansion. Also don't warn about comparisons which are only self + // comparisons within a template specialization. The warnings should catch + // obvious cases in the definition of the template anyways. The idea is to + // warn when the typed comparison operator will always evaluate to the same + // result. + ValueDecl *DL = getCompareDecl(LHSStripped); + ValueDecl *DR = getCompareDecl(RHSStripped); + if (DL && DR && DL == DR && !IsWithinTemplateSpecialization(DL)) { + StringRef Result; + switch (Opc) { + case BO_EQ: case BO_LE: case BO_GE: + Result = "true"; + break; + case BO_NE: case BO_LT: case BO_GT: + Result = "false"; + break; + case BO_Cmp: + Result = "'std::strong_ordering::equal'"; + break; + default: + break; + } + S.DiagRuntimeBehavior(Loc, nullptr, + S.PDiag(diag::warn_comparison_always) + << 0 /*self-comparison*/ << !Result.empty() + << Result); + } else if (DL && DR && LHSType->isArrayType() && RHSType->isArrayType() && + !DL->getType()->isReferenceType() && + !DR->getType()->isReferenceType()) { + // What is it always going to evaluate to? + // FIXME: This is wrong if DL and DR are different Decls for the same + // entity. It's also wrong if DL and/or DR are weak declarations. + StringRef Result; + switch(Opc) { + case BO_EQ: // e.g. array1 == array2 + Result = "false"; + break; + case BO_NE: // e.g. array1 != array2 + Result = "true"; + break; + default: // e.g. array1 <= array2 + // The best we can say is 'a constant' + break; + } + S.DiagRuntimeBehavior(Loc, nullptr, + S.PDiag(diag::warn_comparison_always) + << 1 /*array comparison*/ + << !Result.empty() << Result); + } + + if (isa(LHSStripped)) + LHSStripped = LHSStripped->IgnoreParenCasts(); + if (isa(RHSStripped)) + RHSStripped = RHSStripped->IgnoreParenCasts(); + + // Warn about comparisons against a string constant (unless the other + // operand is null); the user probably wants strcmp. + Expr *LiteralString = nullptr; + Expr *LiteralStringStripped = nullptr; + if ((isa(LHSStripped) || isa(LHSStripped)) && + !RHSStripped->isNullPointerConstant(S.Context, + Expr::NPC_ValueDependentIsNull)) { + LiteralString = LHS; + LiteralStringStripped = LHSStripped; + } else if ((isa(RHSStripped) || + isa(RHSStripped)) && + !LHSStripped->isNullPointerConstant(S.Context, + Expr::NPC_ValueDependentIsNull)) { + LiteralString = RHS; + LiteralStringStripped = RHSStripped; + } + + if (LiteralString) { + S.DiagRuntimeBehavior(Loc, nullptr, + S.PDiag(diag::warn_stringcompare) + << isa(LiteralStringStripped) + << LiteralString->getSourceRange()); + } +} + // C99 6.5.8, C++ [expr.rel] QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, BinaryOperatorKind Opc, @@ -9609,91 +9709,14 @@ QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS, // Handle vector comparisons separately. if (LHS.get()->getType()->isVectorType() || RHS.get()->getType()->isVectorType()) - return CheckVectorCompareOperands(LHS, RHS, Loc, IsRelational); + return CheckVectorCompareOperands(LHS, RHS, Loc, Opc); QualType LHSType = LHS.get()->getType(); QualType RHSType = RHS.get()->getType(); - Expr *LHSStripped = LHS.get()->IgnoreParenImpCasts(); - Expr *RHSStripped = RHS.get()->IgnoreParenImpCasts(); - checkEnumComparison(*this, Loc, LHS.get(), RHS.get()); diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc); - - if (!LHSType->hasFloatingRepresentation() && - !(LHSType->isBlockPointerType() && IsRelational) && - !LHS.get()->getLocStart().isMacroID() && - !RHS.get()->getLocStart().isMacroID() && - !inTemplateInstantiation()) { - // For non-floating point types, check for self-comparisons of the form - // x == x, x != x, x < x, etc. These always evaluate to a constant, and - // often indicate logic errors in the program. - // - // NOTE: Don't warn about comparison expressions resulting from macro - // expansion. Also don't warn about comparisons which are only self - // comparisons within a template specialization. The warnings should catch - // obvious cases in the definition of the template anyways. The idea is to - // warn when the typed comparison operator will always evaluate to the same - // result. - ValueDecl *DL = getCompareDecl(LHSStripped); - ValueDecl *DR = getCompareDecl(RHSStripped); - if (DL && DR && DL == DR && !IsWithinTemplateSpecialization(DL)) { - DiagRuntimeBehavior(Loc, nullptr, PDiag(diag::warn_comparison_always) - << 0 // self- - << (Opc == BO_EQ - || Opc == BO_LE - || Opc == BO_GE)); - } else if (DL && DR && LHSType->isArrayType() && RHSType->isArrayType() && - !DL->getType()->isReferenceType() && - !DR->getType()->isReferenceType()) { - // what is it always going to eval to? - char always_evals_to; - switch(Opc) { - case BO_EQ: // e.g. array1 == array2 - always_evals_to = 0; // false - break; - case BO_NE: // e.g. array1 != array2 - always_evals_to = 1; // true - break; - default: - // best we can say is 'a constant' - always_evals_to = 2; // e.g. array1 <= array2 - break; - } - DiagRuntimeBehavior(Loc, nullptr, PDiag(diag::warn_comparison_always) - << 1 // array - << always_evals_to); - } - - if (isa(LHSStripped)) - LHSStripped = LHSStripped->IgnoreParenCasts(); - if (isa(RHSStripped)) - RHSStripped = RHSStripped->IgnoreParenCasts(); - - // Warn about comparisons against a string constant (unless the other - // operand is null), the user probably wants strcmp. - Expr *literalString = nullptr; - Expr *literalStringStripped = nullptr; - if ((isa(LHSStripped) || isa(LHSStripped)) && - !RHSStripped->isNullPointerConstant(Context, - Expr::NPC_ValueDependentIsNull)) { - literalString = LHS.get(); - literalStringStripped = LHSStripped; - } else if ((isa(RHSStripped) || - isa(RHSStripped)) && - !LHSStripped->isNullPointerConstant(Context, - Expr::NPC_ValueDependentIsNull)) { - literalString = RHS.get(); - literalStringStripped = RHSStripped; - } - - if (literalString) { - DiagRuntimeBehavior(Loc, nullptr, - PDiag(diag::warn_stringcompare) - << isa(literalStringStripped) - << literalString->getSourceRange()); - } - } + diagnoseTautologicalComparison(*this, Loc, LHS.get(), RHS.get(), Opc); // C99 6.5.8p3 / C99 6.5.9p4 UsualArithmeticConversions(LHS, RHS); @@ -10101,7 +10124,7 @@ QualType Sema::GetSignedVectorType(QualType V) { /// types. QualType Sema::CheckVectorCompareOperands(ExprResult &LHS, ExprResult &RHS, SourceLocation Loc, - bool IsRelational) { + BinaryOperatorKind Opc) { // Check to make sure we're operating on vectors of the same type and width, // Allowing one side to be a scalar of element type. QualType vType = CheckVectorOperands(LHS, RHS, Loc, /*isCompAssign*/false, @@ -10121,22 +10144,12 @@ QualType Sema::CheckVectorCompareOperands(ExprResult &LHS, ExprResult &RHS, // For non-floating point types, check for self-comparisons of the form // x == x, x != x, x < x, etc. These always evaluate to a constant, and // often indicate logic errors in the program. - if (!LHSType->hasFloatingRepresentation() && !inTemplateInstantiation()) { - if (DeclRefExpr* DRL - = dyn_cast(LHS.get()->IgnoreParenImpCasts())) - if (DeclRefExpr* DRR - = dyn_cast(RHS.get()->IgnoreParenImpCasts())) - if (DRL->getDecl() == DRR->getDecl()) - DiagRuntimeBehavior(Loc, nullptr, - PDiag(diag::warn_comparison_always) - << 0 // self- - << 2 // "a constant" - ); - } + diagnoseTautologicalComparison(*this, Loc, LHS.get(), RHS.get(), Opc); // Check for comparisons of floating point operands using != and ==. - if (!IsRelational && LHSType->hasFloatingRepresentation()) { - assert (RHS.get()->getType()->hasFloatingRepresentation()); + if (BinaryOperator::isEqualityOp(Opc) && + LHSType->hasFloatingRepresentation()) { + assert(RHS.get()->getType()->hasFloatingRepresentation()); CheckFloatComparison(Loc, LHS.get(), RHS.get()); } diff --git a/clang/test/Sema/ext_vector_comparisons.c b/clang/test/Sema/ext_vector_comparisons.c index 605ba6c55e33..4c632a412f44 100644 --- a/clang/test/Sema/ext_vector_comparisons.c +++ b/clang/test/Sema/ext_vector_comparisons.c @@ -6,12 +6,12 @@ static int4 test1() { int4 vec, rv; // comparisons to self... - return vec == vec; // expected-warning{{self-comparison always evaluates to a constant}} - return vec != vec; // expected-warning{{self-comparison always evaluates to a constant}} - return vec < vec; // expected-warning{{self-comparison always evaluates to a constant}} - return vec <= vec; // expected-warning{{self-comparison always evaluates to a constant}} - return vec > vec; // expected-warning{{self-comparison always evaluates to a constant}} - return vec >= vec; // expected-warning{{self-comparison always evaluates to a constant}} + return vec == vec; // expected-warning{{self-comparison always evaluates to true}} + return vec != vec; // expected-warning{{self-comparison always evaluates to false}} + return vec < vec; // expected-warning{{self-comparison always evaluates to false}} + return vec <= vec; // expected-warning{{self-comparison always evaluates to true}} + return vec > vec; // expected-warning{{self-comparison always evaluates to false}} + return vec >= vec; // expected-warning{{self-comparison always evaluates to true}} }