Author: rsmith Date: Wed Dec 6 11:23:19 2017 New Revision: 319942 URL: http://llvm.org/viewvc/llvm-project?rev=319942&view=rev Log: Delete special-case "out-of-range" handling for bools, and just use the normal codepath plus the new "minimum / maximum value of type" diagnostic to get the same effect.
Move the warning for an in-range but tautological comparison of a constant (0 or 1) against a bool out of -Wtautological-constant-out-of-range-compare into the more-appropriate -Wtautological-constant-compare. Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td cfe/trunk/lib/Sema/SemaChecking.cpp Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=319942&r1=319941&r2=319942&view=diff ============================================================================== --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Dec 6 11:23:19 2017 @@ -5952,6 +5952,8 @@ def warn_out_of_range_compare : Warning< "comparison of %select{constant %0|true|false}1 with " "%select{expression of type %2|boolean expression}3 is always " "%select{false|true}4">, InGroup<TautologicalOutOfRangeCompare>; +def warn_tautological_bool_compare : Warning<warn_out_of_range_compare.Text>, + InGroup<TautologicalConstantCompare>; def warn_comparison_of_mixed_enum_types : Warning< "comparison of two values with different enumeration types" "%diff{ ($ and $)|}0,1">, Modified: cfe/trunk/lib/Sema/SemaChecking.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=319942&r1=319941&r2=319942&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) +++ cfe/trunk/lib/Sema/SemaChecking.cpp Wed Dec 6 11:23:19 2017 @@ -8650,15 +8650,8 @@ static bool IsEnumConstOrFromMacro(Sema return false; } -static bool isNonBooleanIntegerValue(Expr *E) { - return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType(); -} - -static bool isNonBooleanUnsignedValue(Expr *E) { - // We are checking that the expression is not known to have boolean value, - // is an integer type; and is either unsigned after implicit casts, - // or was unsigned before implicit casts. - return isNonBooleanIntegerValue(E) && +static bool isKnownToHaveUnsignedValue(Expr *E) { + return E->getType()->isIntegerType() && (!E->getType()->isSignedIntegerType() || !E->IgnoreParenImpCasts()->getType()->isSignedIntegerType()); } @@ -8684,7 +8677,7 @@ static llvm::Optional<LimitType> IsTypeL if (IsEnumConstOrFromMacro(S, Constant)) return llvm::Optional<LimitType>(); - if (isNonBooleanUnsignedValue(Other) && Value == 0) + if (isKnownToHaveUnsignedValue(Other) && Value == 0) return LimitType::Min; // TODO: Investigate using GetExprRange() to get tighter bounds @@ -8694,20 +8687,20 @@ static llvm::Optional<LimitType> IsTypeL OtherT = AT->getValueType(); IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT); + if (Other->isKnownToHaveBooleanValue()) + OtherRange = IntRange::forBoolType(); // Special-case for C++ for enum with one enumerator with value of 0. if (OtherRange.Width == 0) return Value == 0 ? LimitType::Both : llvm::Optional<LimitType>(); if (llvm::APSInt::isSameValue( - llvm::APSInt::getMaxValue(OtherRange.Width, - OtherT->isUnsignedIntegerType()), + llvm::APSInt::getMaxValue(OtherRange.Width, OtherRange.NonNegative), Value)) return LimitType::Max; if (llvm::APSInt::isSameValue( - llvm::APSInt::getMinValue(OtherRange.Width, - OtherT->isUnsignedIntegerType()), + llvm::APSInt::getMinValue(OtherRange.Width, OtherRange.NonNegative), Value)) return LimitType::Min; @@ -8726,6 +8719,20 @@ static bool HasEnumType(Expr *E) { return E->getType()->isEnumeralType(); } +static int classifyConstantValue(Expr *Constant) { + // The values of this enumeration are used in the diagnostics + // diag::warn_out_of_range_compare and diag::warn_tautological_bool_compare. + enum ConstantValueKind { + Miscellaneous = 0, + LiteralTrue, + LiteralFalse + }; + if (auto *BL = dyn_cast<CXXBoolLiteralExpr>(Constant)) + return BL->getValue() ? ConstantValueKind::LiteralTrue + : ConstantValueKind::LiteralFalse; + return ConstantValueKind::Miscellaneous; +} + static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E, Expr *Constant, Expr *Other, const llvm::APSInt &Value, @@ -8738,11 +8745,12 @@ static bool CheckTautologicalComparison( BinaryOperatorKind Op = E->getOpcode(); QualType OType = Other->IgnoreParenImpCasts()->getType(); + if (!OType->isIntegerType()) + return false; - llvm::Optional<LimitType> ValueType; // Which limit (min/max) is the constant? - - if (!(isNonBooleanIntegerValue(Other) && - (ValueType = IsTypeLimit(S, Constant, Other, Value)))) + // Determine which limit (min/max) the constant is, if either. + llvm::Optional<LimitType> ValueType = IsTypeLimit(S, Constant, Other, Value); + if (!ValueType) return false; bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ RhsConstant; @@ -8757,17 +8765,30 @@ static bool CheckTautologicalComparison( const bool Result = ResultWhenConstEqualsOther; - unsigned Diag = (isNonBooleanUnsignedValue(Other) && Value == 0) - ? (HasEnumType(Other) - ? diag::warn_unsigned_enum_always_true_comparison - : diag::warn_unsigned_always_true_comparison) - : diag::warn_tautological_constant_compare; - // Should be enough for uint128 (39 decimal digits) SmallString<64> PrettySourceValue; llvm::raw_svector_ostream OS(PrettySourceValue); OS << Value; + // FIXME: We use a somewhat different formatting for the cases involving + // boolean values for historical reasons. We should pick a consistent way + // of presenting these diagnostics. + if (Other->isKnownToHaveBooleanValue()) { + S.DiagRuntimeBehavior( + E->getOperatorLoc(), E, + S.PDiag(diag::warn_tautological_bool_compare) + << OS.str() << classifyConstantValue(Constant->IgnoreParenImpCasts()) + << OType << !OType->isBooleanType() << Result + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); + return true; + } + + unsigned Diag = (isKnownToHaveUnsignedValue(Other) && Value == 0) + ? (HasEnumType(Other) + ? diag::warn_unsigned_enum_always_true_comparison + : diag::warn_unsigned_always_true_comparison) + : diag::warn_tautological_constant_compare; + S.Diag(E->getOperatorLoc(), Diag) << RhsConstant << OType << E->getOpcodeStr() << OS.str() << Result << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); @@ -8791,26 +8812,29 @@ static bool DiagnoseOutOfRangeComparison QualType OtherT = Other->getType(); if (const auto *AT = OtherT->getAs<AtomicType>()) OtherT = AT->getValueType(); + IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT); - unsigned OtherWidth = OtherRange.Width; - bool OtherIsBooleanType = Other->isKnownToHaveBooleanValue(); + // Whether we're treating Other as being a bool because of the form of + // expression despite it having another type (typically 'int' in C). + bool OtherIsBooleanDespiteType = + !OtherT->isBooleanType() && Other->isKnownToHaveBooleanValue(); + if (OtherIsBooleanDespiteType) + OtherRange = IntRange::forBoolType(); + + unsigned OtherWidth = OtherRange.Width; BinaryOperatorKind op = E->getOpcode(); bool IsTrue = true; - // Used for diagnostic printout. - enum { - LiteralConstant = 0, - CXXBoolLiteralTrue, - CXXBoolLiteralFalse - } LiteralOrBoolConstant = LiteralConstant; - - if (!OtherIsBooleanType) { + // Check whether the constant value can be represented in OtherRange. Bail + // out if so; this isn't an out-of-range comparison. + { QualType ConstantT = Constant->getType(); QualType CommonT = E->getLHS()->getType(); - if (S.Context.hasSameUnqualifiedType(OtherT, ConstantT)) + if (S.Context.hasSameUnqualifiedType(OtherT, ConstantT) && + !OtherIsBooleanDespiteType) return false; assert((OtherT->isIntegerType() && ConstantT->isIntegerType()) && "comparison with non-integer type"); @@ -8882,84 +8906,6 @@ static bool DiagnoseOutOfRangeComparison else // op == BO_GT || op == BO_GE IsTrue = PositiveConstant; } - } else { - // Other isKnownToHaveBooleanValue - enum CompareBoolWithConstantResult { AFals, ATrue, Unkwn }; - enum ConstantValue { LT_Zero, Zero, One, GT_One, SizeOfConstVal }; - enum ConstantSide { Lhs, Rhs, SizeOfConstSides }; - - static const struct LinkedConditions { - CompareBoolWithConstantResult BO_LT_OP[SizeOfConstSides][SizeOfConstVal]; - CompareBoolWithConstantResult BO_GT_OP[SizeOfConstSides][SizeOfConstVal]; - CompareBoolWithConstantResult BO_LE_OP[SizeOfConstSides][SizeOfConstVal]; - CompareBoolWithConstantResult BO_GE_OP[SizeOfConstSides][SizeOfConstVal]; - CompareBoolWithConstantResult BO_EQ_OP[SizeOfConstSides][SizeOfConstVal]; - CompareBoolWithConstantResult BO_NE_OP[SizeOfConstSides][SizeOfConstVal]; - - } TruthTable = { - // Constant on LHS. | Constant on RHS. | - // LT_Zero| Zero | One |GT_One| LT_Zero| Zero | One |GT_One| - { { ATrue, Unkwn, AFals, AFals }, { AFals, AFals, Unkwn, ATrue } }, - { { AFals, AFals, Unkwn, ATrue }, { ATrue, Unkwn, AFals, AFals } }, - { { ATrue, ATrue, Unkwn, AFals }, { AFals, Unkwn, ATrue, ATrue } }, - { { AFals, Unkwn, ATrue, ATrue }, { ATrue, ATrue, Unkwn, AFals } }, - { { AFals, Unkwn, Unkwn, AFals }, { AFals, Unkwn, Unkwn, AFals } }, - { { ATrue, Unkwn, Unkwn, ATrue }, { ATrue, Unkwn, Unkwn, ATrue } } - }; - - bool ConstantIsBoolLiteral = isa<CXXBoolLiteralExpr>(Constant); - - enum ConstantValue ConstVal = Zero; - if (Value.isUnsigned() || Value.isNonNegative()) { - if (Value == 0) { - LiteralOrBoolConstant = - ConstantIsBoolLiteral ? CXXBoolLiteralFalse : LiteralConstant; - ConstVal = Zero; - } else if (Value == 1) { - LiteralOrBoolConstant = - ConstantIsBoolLiteral ? CXXBoolLiteralTrue : LiteralConstant; - ConstVal = One; - } else { - LiteralOrBoolConstant = LiteralConstant; - ConstVal = GT_One; - } - } else { - ConstVal = LT_Zero; - } - - CompareBoolWithConstantResult CmpRes; - - switch (op) { - case BO_LT: - CmpRes = TruthTable.BO_LT_OP[RhsConstant][ConstVal]; - break; - case BO_GT: - CmpRes = TruthTable.BO_GT_OP[RhsConstant][ConstVal]; - break; - case BO_LE: - CmpRes = TruthTable.BO_LE_OP[RhsConstant][ConstVal]; - break; - case BO_GE: - CmpRes = TruthTable.BO_GE_OP[RhsConstant][ConstVal]; - break; - case BO_EQ: - CmpRes = TruthTable.BO_EQ_OP[RhsConstant][ConstVal]; - break; - case BO_NE: - CmpRes = TruthTable.BO_NE_OP[RhsConstant][ConstVal]; - break; - default: - CmpRes = Unkwn; - break; - } - - if (CmpRes == AFals) { - IsTrue = false; - } else if (CmpRes == ATrue) { - IsTrue = true; - } else { - return false; - } } // If this is a comparison to an enum constant, include that @@ -8978,8 +8924,8 @@ static bool DiagnoseOutOfRangeComparison S.DiagRuntimeBehavior( E->getOperatorLoc(), E, S.PDiag(diag::warn_out_of_range_compare) - << OS.str() << LiteralOrBoolConstant - << OtherT << (OtherIsBooleanType && !OtherT->isBooleanType()) << IsTrue + << OS.str() << classifyConstantValue(Constant) + << OtherT << OtherIsBooleanDespiteType << IsTrue << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); return true; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits