I've reverted in r320133 since it caused new warnings in Chromium that I'm not sure were intentional. See comment on the revert.
On Thu, Dec 7, 2017 at 5:00 PM, Richard Smith via cfe-commits <cfe-commits@lists.llvm.org> wrote: > Author: rsmith > Date: Thu Dec 7 17:00:27 2017 > New Revision: 320124 > > URL: http://llvm.org/viewvc/llvm-project?rev=320124&view=rev > Log: > Fold together the in-range and out-of-range portions of > -Wtautological-compare. > > Modified: > cfe/trunk/lib/Sema/SemaChecking.cpp > > Modified: cfe/trunk/lib/Sema/SemaChecking.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=320124&r1=320123&r2=320124&view=diff > ============================================================================== > --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) > +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec 7 17:00:27 2017 > @@ -8801,12 +8801,7 @@ static bool CheckTautologicalComparison( > Expr *Constant, Expr *Other, > const llvm::APSInt &Value, > bool RhsConstant) { > - // Disable warning in template instantiations > - // and only analyze <, >, <= and >= operations. > - if (S.inTemplateInstantiation() || !E->isRelationalOp()) > - return false; > - > - if (IsEnumConstOrFromMacro(S, Constant)) > + if (S.inTemplateInstantiation()) > return false; > > Expr *OriginalOther = Other; > @@ -8833,94 +8828,23 @@ static bool CheckTautologicalComparison( > OtherRange.Width = > std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width); > > - // Check whether the constant value can be represented in OtherRange. Bail > - // out if so; this isn't an out-of-range comparison. > + // Determine the promoted range of the other type and see if a comparison > of > + // the constant against that range is tautological. > PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(), > Value.isUnsigned()); > - > auto Cmp = OtherPromotedRange.compare(Value); > - if (Cmp != PromotedRange::Min && Cmp != PromotedRange::Max && > - Cmp != PromotedRange::OnlyValue) > - return false; > - > auto Result = PromotedRange::constantValue(E->getOpcode(), Cmp, > RhsConstant); > if (!Result) > return false; > > - // 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) > - << OtherT << !OtherT->isBooleanType() << *Result > - << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); > - return true; > - } > - > - unsigned Diag = (isKnownToHaveUnsignedValue(OriginalOther) && Value == 0) > - ? (HasEnumType(OriginalOther) > - ? > diag::warn_unsigned_enum_always_true_comparison > - : diag::warn_unsigned_always_true_comparison) > - : diag::warn_tautological_constant_compare; > - > - S.Diag(E->getOperatorLoc(), Diag) > - << RhsConstant << OtherT << E->getOpcodeStr() << OS.str() << *Result > - << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); > - > - return true; > -} > - > -static bool DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, > - Expr *Constant, Expr *Other, > - const llvm::APSInt &Value, > - bool RhsConstant) { > - // Disable warning in template instantiations. > - if (S.inTemplateInstantiation()) > - return false; > - > - Constant = Constant->IgnoreParenImpCasts(); > - Other = Other->IgnoreParenImpCasts(); > - > - // TODO: Investigate using GetExprRange() to get tighter bounds > - // on the bit ranges. > - QualType OtherT = Other->getType(); > - if (const auto *AT = OtherT->getAs<AtomicType>()) > - OtherT = AT->getValueType(); > - IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT); > - > - // 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(); > - > - if (FieldDecl *Bitfield = Other->getSourceBitField()) > - if (!Bitfield->getBitWidth()->isValueDependent()) > - OtherRange.Width = > - std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width); > - > - // Check whether the constant value can be represented in OtherRange. Bail > - // out if so; this isn't an out-of-range comparison. > - PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(), > - Value.isUnsigned()); > - auto Cmp = OtherPromotedRange.compare(Value); > - > - // If Value is in the range of possible Other values, this comparison is > not > - // tautological. > - if (Cmp & PromotedRange::InRangeFlag) > - return false; > - > - auto IsTrue = PromotedRange::constantValue(E->getOpcode(), Cmp, > RhsConstant); > - if (!IsTrue) > + // Suppress the diagnostic for an in-range comparison if the constant comes > + // from a macro or enumerator. We don't want to diagnose > + // > + // some_long_value <= INT_MAX > + // > + // when sizeof(int) == sizeof(long). > + bool InRange = Cmp & PromotedRange::InRangeFlag; > + if (InRange && IsEnumConstOrFromMacro(S, Constant)) > return false; > > // If this is a comparison to an enum constant, include that > @@ -8929,6 +8853,7 @@ static bool DiagnoseOutOfRangeComparison > if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Constant)) > ED = dyn_cast<EnumConstantDecl>(DR->getDecl()); > > + // Should be enough for uint128 (39 decimal digits) > SmallString<64> PrettySourceValue; > llvm::raw_svector_ostream OS(PrettySourceValue); > if (ED) > @@ -8936,14 +8861,30 @@ static bool DiagnoseOutOfRangeComparison > else > OS << Value; > > - S.DiagRuntimeBehavior( > - E->getOperatorLoc(), E, > - S.PDiag(diag::warn_out_of_range_compare) > - << OS.str() << classifyConstantValue(Constant) > - << OtherT << OtherIsBooleanDespiteType << *IsTrue > - << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); > + // FIXME: We use a somewhat different formatting for the in-range cases and > + // cases involving boolean values for historical reasons. We should pick a > + // consistent way of presenting these diagnostics. > + if (!InRange || Other->isKnownToHaveBooleanValue()) { > + S.DiagRuntimeBehavior( > + E->getOperatorLoc(), E, > + S.PDiag(!InRange ? diag::warn_out_of_range_compare > + : diag::warn_tautological_bool_compare) > + << OS.str() << classifyConstantValue(Constant) > + << OtherT << OtherIsBooleanDespiteType << *Result > + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); > + } else { > + unsigned Diag = (isKnownToHaveUnsignedValue(OriginalOther) && Value == 0) > + ? (HasEnumType(OriginalOther) > + ? > diag::warn_unsigned_enum_always_true_comparison > + : diag::warn_unsigned_always_true_comparison) > + : diag::warn_tautological_constant_compare; > + > + S.Diag(E->getOperatorLoc(), Diag) > + << RhsConstant << OtherT << E->getOpcodeStr() << OS.str() << *Result > + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); > + } > > - return true; > + return true; > } > > /// Analyze the operands of the given comparison. Implements the > @@ -8993,12 +8934,8 @@ static void AnalyzeComparison(Sema &S, B > > // Check whether an integer constant comparison results in a value > // of 'true' or 'false'. > - > if (CheckTautologicalComparison(S, E, Const, Other, Value, > RhsConstant)) > return AnalyzeImpConvsInComparison(S, E); > - > - if (DiagnoseOutOfRangeComparison(S, E, Const, Other, Value, > RhsConstant)) > - return AnalyzeImpConvsInComparison(S, E); > } > } > > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
