# Changeset View

# Standalone View

# lib/Sema/SemaChecking.cpp

- This file is larger than 256 KB, so syntax highlighting is disabled by default.

Show First 20 Lines • Show All 8547 Lines • ▼ Show 20 Line(s) | 8536 | bool IsSameFloatAfterCast(const APValue &value, | |||
---|---|---|---|---|---|

8548 | 8548 | | |||

8549 | assert(value.isComplexFloat()); | 8549 | assert(value.isComplexFloat()); | ||

8550 | return (IsSameFloatAfterCast(value.getComplexFloatReal(), Src, Tgt) && | 8550 | return (IsSameFloatAfterCast(value.getComplexFloatReal(), Src, Tgt) && | ||

8551 | IsSameFloatAfterCast(value.getComplexFloatImag(), Src, Tgt)); | 8551 | IsSameFloatAfterCast(value.getComplexFloatImag(), Src, Tgt)); | ||

8552 | } | 8552 | } | ||

8553 | 8553 | | |||

8554 | void AnalyzeImplicitConversions(Sema &S, Expr *E, SourceLocation CC); | 8554 | void AnalyzeImplicitConversions(Sema &S, Expr *E, SourceLocation CC); | ||

8555 | 8555 | | |||

8556 | bool IsZero(Sema &S, Expr *E) { | 8556 | bool IsEnumConstOrFromMacro(Sema &S, Expr *E) { | ||

8557 | // Suppress cases where we are comparing against an enum constant. | 8557 | // Suppress cases where we are comparing against an enum constant. | ||

8558 | if (const DeclRefExpr *DR = | 8558 | if (const DeclRefExpr *DR = | ||

8559 | dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts())) | 8559 | dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts())) | ||

8560 | if (isa<EnumConstantDecl>(DR->getDecl())) | 8560 | if (isa<EnumConstantDecl>(DR->getDecl())) | ||

8561 | return false; | 8561 | return true; | ||

8562 | 8562 | | |||

8563 | // Suppress cases where the '0' value is expanded from a macro. | 8563 | // Suppress cases where the '0' value is expanded from a macro. | ||

8564 | if (E->getLocStart().isMacroID()) | 8564 | if (E->getLocStart().isMacroID()) | ||

8565 | return true; | ||||

8566 | | ||||

8565 | return false; | 8567 | return false; | ||

8568 | } | ||||

8566 | 8569 | | |||

8567 | llvm::APSInt Value; | 8570 | bool isNonBooleanIntegerValue(Expr *E) { | ||

8568 | return E->isIntegerConstantExpr(Value, S.Context) && Value == 0; | 8571 | return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType(); | ||

8572 | } | ||||

8573 | | ||||

8574 | bool isNonBooleanUnsignedValue(Expr *E) { | ||||

8575 | // We are checking that the expression is not known to have boolean value, | ||||

8576 | // is an integer type; and is either unsigned after implicit casts, | ||||

8577 | // or was unsigned before implicit casts. | ||||

8578 | return isNonBooleanIntegerValue(E) && | ||||

8579 | (!E->getType()->isSignedIntegerType() || | ||||

8580 | !E->IgnoreParenImpCasts()->getType()->isSignedIntegerType()); | ||||

8581 | } | ||||

8582 | | ||||

8583 | enum class LimitType { | ||||

8584 | Max, // e.g. 32767 for short | ||||

8585 | Min // e.g. -32768 for short | ||||

8586 | }; | ||||

8587 | | ||||

8588 | /// Checks whether Expr 'Constant' may be the | ||||

8589 | /// std::numeric_limits<>::max() or std::numeric_limits<>::min() | ||||

8590 | /// of the Expr 'Other'. If true, then returns the limit type (min or max). | ||||

8591 | /// The Value is the evaluation of Constant | ||||

8592 | llvm::Optional<LimitType> IsTypeLimit(Sema &S, Expr *Constant, Expr *Other, | ||||

8593 | const llvm::APSInt &Value) { | ||||

8594 | if (IsEnumConstOrFromMacro(S, Constant)) | ||||

8595 | return llvm::Optional<LimitType>(); | ||||

8596 | | ||||

8597 | if (isNonBooleanUnsignedValue(Other) && Value == 0) | ||||

8598 | return LimitType::Min; | ||||

8599 | | ||||

8600 | // TODO: Investigate using GetExprRange() to get tighter bounds | ||||

8601 | // on the bit ranges. | ||||

8602 | QualType OtherT = Other->IgnoreParenImpCasts()->getType(); | ||||

8603 | if (const auto *AT = OtherT->getAs<AtomicType>()) | ||||

8604 | OtherT = AT->getValueType(); | ||||

8605 | | ||||

8606 | IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT); | ||||

8607 | | ||||

8608 | if (llvm::APSInt::isSameValue( | ||||

8609 | llvm::APSInt::getMaxValue(OtherRange.Width, | ||||

8610 | OtherT->isUnsignedIntegerType()), | ||||

8611 | Value)) | ||||

8612 | return LimitType::Max; | ||||

8613 | | ||||

8614 | if (llvm::APSInt::isSameValue( | ||||

8615 | llvm::APSInt::getMinValue(OtherRange.Width, | ||||

8616 | OtherT->isUnsignedIntegerType()), | ||||

8617 | Value)) | ||||

8618 | return LimitType::Min; | ||||

8619 | | ||||

8620 | return llvm::Optional<LimitType>(); | ||||

8569 | } | 8621 | } | ||

8570 | 8622 | | |||

8571 | bool HasEnumType(Expr *E) { | 8623 | bool HasEnumType(Expr *E) { | ||

8572 | // Strip off implicit integral promotions. | 8624 | // Strip off implicit integral promotions. | ||

8573 | while (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) { | 8625 | while (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) { | ||

8574 | if (ICE->getCastKind() != CK_IntegralCast && | 8626 | if (ICE->getCastKind() != CK_IntegralCast && | ||

8575 | ICE->getCastKind() != CK_NoOp) | 8627 | ICE->getCastKind() != CK_NoOp) | ||

8576 | break; | 8628 | break; | ||

8577 | E = ICE->getSubExpr(); | 8629 | E = ICE->getSubExpr(); | ||

8578 | } | 8630 | } | ||

8579 | 8631 | | |||

rsmith: Use `///` for documentation comments. | |||||

Done ReplyIs there a reason to separate the checks for 0 from the other checks for min/max values? It looks like the code below would be slightly simpler if the two checks were combined. rsmith: Is there a reason to separate the checks for 0 from the other checks for min/max values? It… | |||||

8580 | return E->getType()->isEnumeralType(); | 8632 | return E->getType()->isEnumeralType(); | ||

8581 | } | 8633 | } | ||

Done Reply
rsmith: `Value` can't be null; pass it by reference instead. | |||||

8582 | 8634 | | |||

8583 | bool isNonBooleanUnsignedValue(Expr *E) { | 8635 | bool CheckTautologicalComparison(Sema &S, BinaryOperator *E, Expr *Constant, | ||

8584 | // We are checking that the expression is not known to have boolean value, | 8636 | Expr *Other, const llvm::APSInt &Value, | ||

8585 | // is an integer type; and is either unsigned after implicit casts, | 8637 | bool RhsConstant) { | ||

Done ReplyThis comment adds nothing to the following code; remove? rsmith: This comment adds nothing to the following code; remove? | |||||

8586 | // or was unsigned before implicit casts. | 8638 | // Disable warning in template instantiations | ||

8587 | return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType() && | 8639 | // and only analyze <, >, <= and >= operations. | ||

8588 | (!E->getType()->isSignedIntegerType() || | 8640 | if (S.inTemplateInstantiation() || !E->isRelationalOp()) | ||

8589 | !E->IgnoreParenImpCasts()->getType()->isSignedIntegerType()); | | |||

8590 | } | | |||

8591 | | ||||

8592 | bool CheckTautologicalComparisonWithZero(Sema &S, BinaryOperator *E) { | | |||

8593 | // Disable warning in template instantiations. | | |||

8594 | if (S.inTemplateInstantiation()) | | |||

8595 | return false; | 8641 | return false; | ||

8596 | 8642 | | |||

8597 | // bool values are handled by DiagnoseOutOfRangeComparison(). | | |||

8598 | | ||||

8599 | BinaryOperatorKind Op = E->getOpcode(); | 8643 | BinaryOperatorKind Op = E->getOpcode(); | ||

8600 | if (E->isValueDependent()) | 8644 | | ||

8645 | QualType OType = Other->IgnoreParenImpCasts()->getType(); | ||||

8646 | | ||||

8647 | llvm::Optional<LimitType> ValueType; // Which limit (min/max) is the constant? | ||||

8648 | | ||||

8649 | if (!(isNonBooleanIntegerValue(Other) && | ||||

8650 | (ValueType = IsTypeLimit(S, Constant, Other, Value)))) | ||||

8601 | return false; | 8651 | return false; | ||

8602 | 8652 | | |||

8603 | Expr *LHS = E->getLHS(); | 8653 | bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ RhsConstant; | ||

8604 | Expr *RHS = E->getRHS(); | 8654 | bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE); | ||

8655 | bool ResultWhenConstNeOther = ConstIsLowerBound ^ ValueType == LimitType::Max; | ||||

8656 | if (ResultWhenConstEqualsOther != ResultWhenConstNeOther) | ||||

8657 | return false; // The comparison is not tautological. | ||||

8605 | 8658 | | |||

8606 | bool Match = true; | 8659 | const bool Result = ResultWhenConstEqualsOther; | ||

8607 | 8660 | | |||

8608 | if (Op == BO_LT && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) { | 8661 | unsigned Diag = (isNonBooleanUnsignedValue(Other) && Value == 0) | ||

8609 | S.Diag(E->getOperatorLoc(), | 8662 | ? (HasEnumType(Other) | ||

8610 | HasEnumType(LHS) ? diag::warn_lunsigned_enum_always_true_comparison | 8663 | ? diag::warn_unsigned_enum_always_true_comparison | ||

8611 | : diag::warn_lunsigned_always_true_comparison) | 8664 | : diag::warn_unsigned_always_true_comparison) | ||

8612 | << "< 0" << false << LHS->getSourceRange() << RHS->getSourceRange(); | 8665 | : diag::warn_tautological_constant_compare; | ||

8613 | } else if (Op == BO_GE && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) { | 8666 | | ||

8614 | S.Diag(E->getOperatorLoc(), | 8667 | // Should be enough for uint128 (39 decimal digits) | ||

8615 | HasEnumType(LHS) ? diag::warn_lunsigned_enum_always_true_comparison | 8668 | SmallString<64> PrettySourceValue; | ||

8616 | : diag::warn_lunsigned_always_true_comparison) | 8669 | llvm::raw_svector_ostream OS(PrettySourceValue); | ||

8617 | << ">= 0" << true << LHS->getSourceRange() << RHS->getSourceRange(); | 8670 | OS << Value; | ||

8618 | } else if (Op == BO_GT && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) { | | |||

8619 | S.Diag(E->getOperatorLoc(), | | |||

8620 | HasEnumType(RHS) ? diag::warn_runsigned_enum_always_true_comparison | | |||

8621 | : diag::warn_runsigned_always_true_comparison) | | |||

8622 | << "0 >" << false << LHS->getSourceRange() << RHS->getSourceRange(); | | |||

8623 | } else if (Op == BO_LE && isNonBooleanUnsignedValue(RHS) && IsZero(S, LHS)) { | | |||

8624 | S.Diag(E->getOperatorLoc(), | | |||

8625 | HasEnumType(RHS) ? diag::warn_runsigned_enum_always_true_comparison | | |||

8626 | : diag::warn_runsigned_always_true_comparison) | | |||

8627 | << "0 <=" << true << LHS->getSourceRange() << RHS->getSourceRange(); | | |||

8628 | } else | | |||

8629 | Match = false; | | |||

8630 | 8671 | | |||

8631 | return Match; | 8672 | S.Diag(E->getOperatorLoc(), Diag) | ||

8673 | << RhsConstant << OType << E->getOpcodeStr() << OS.str() << Result | ||||

8674 | << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); | ||||

8675 | | ||||

8676 | return true; | ||||

8632 | } | 8677 | } | ||

8633 | 8678 | | |||

8634 | void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, Expr *Constant, | 8679 | bool DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, Expr *Constant, | ||

8635 | Expr *Other, const llvm::APSInt &Value, | 8680 | Expr *Other, const llvm::APSInt &Value, | ||

8636 | bool RhsConstant) { | 8681 | bool RhsConstant) { | ||

8637 | // Disable warning in template instantiations. | 8682 | // Disable warning in template instantiations. | ||

8638 | if (S.inTemplateInstantiation()) | 8683 | if (S.inTemplateInstantiation()) | ||

Done ReplyThe exhaustive case analysis is a little hard to verify. Perhaps something like this would be clearer: bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ ConstOnRight; bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE); bool ResultWhenConstNeOther = ConstIsLowerBound ^ ValueType == LimitType::Max; if (ResultWhenConstEqualsOther != ResultWhenConstNeOther) return false; rsmith: The exhaustive case analysis is a little hard to verify. Perhaps something like this would be… | |||||

Not Done ReplyOh, that looks better indeed :) lebedev.ri: Oh, that looks better indeed :)
I did consider doing something like that, but my variant ended… | |||||

8639 | return; | 8684 | return false; | ||

8685 | | ||||

8686 | Constant = Constant->IgnoreParenImpCasts(); | ||||

8687 | Other = Other->IgnoreParenImpCasts(); | ||||

8640 | 8688 | | |||

8641 | // TODO: Investigate using GetExprRange() to get tighter bounds | 8689 | // TODO: Investigate using GetExprRange() to get tighter bounds | ||

8642 | // on the bit ranges. | 8690 | // on the bit ranges. | ||

8643 | QualType OtherT = Other->getType(); | 8691 | QualType OtherT = Other->getType(); | ||

8644 | if (const auto *AT = OtherT->getAs<AtomicType>()) | 8692 | if (const auto *AT = OtherT->getAs<AtomicType>()) | ||

8645 | OtherT = AT->getValueType(); | 8693 | OtherT = AT->getValueType(); | ||

8646 | IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT); | 8694 | IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT); | ||

8647 | unsigned OtherWidth = OtherRange.Width; | 8695 | unsigned OtherWidth = OtherRange.Width; | ||

8648 | 8696 | | |||

8649 | bool OtherIsBooleanType = Other->isKnownToHaveBooleanValue(); | 8697 | bool OtherIsBooleanType = Other->isKnownToHaveBooleanValue(); | ||

8650 | 8698 | | |||

8651 | // 0 values are handled later by CheckTautologicalComparisonWithZero(). | | |||

8652 | if ((Value == 0) && (!OtherIsBooleanType)) | | |||

8653 | return; | | |||

8654 | | ||||

8655 | BinaryOperatorKind op = E->getOpcode(); | 8699 | BinaryOperatorKind op = E->getOpcode(); | ||

Done ReplyIf the diagnostic we are about to output is disabled, should we still lebedev.ri: If the diagnostic we are about to output is disabled, should we still `return true;` and… | |||||

Done ReplyThe general principle is to behave as if we produced all diagnostics, and then filtered out the ones that aren't enabled. So if a more specialized (eg tautological comparison) diagnostic is disabled, the more general (eg sign compare) diagnostic should not appear. In short, this is fine :) rsmith: The general principle is to behave as if we produced all diagnostics, and then filtered out the… | |||||

8656 | bool IsTrue = true; | 8700 | bool IsTrue = true; | ||

Done ReplyComments should start with a capital letter. rsmith: Comments should start with a capital letter. | |||||

Not Done ReplyIs this necessary? (Aren't the type limit values always within the range of the type in question?) Can we avoid evaluating rsmith: Is this necessary? (Aren't the type limit values always within the range of the type in… | |||||

Not Done ReplyUhm, good question :) lebedev.ri: Uhm, good question :)
If i remove this, `check-clang-sema` and `check-clang-semacxx` still pass. | |||||

Not Done ReplyThe most original version of this code - Either simply restore the original check:
// 0 values are handled later by CheckTautologicalComparison(). if ((Value == 0) && (!OtherIsBooleanType)) return; And add a comment there about it - Or, drop it completely
- Or, perhaps refactor
`CheckTautologicalComparison()`, and more or less call it from function`AnalyzeComparison()`, before calling`DiagnoseOutOfRangeComparison()`, thus completely avoiding the need to re-evaluate`Constant`there later on, and simplify the logic in the process.
I personally think the lebedev.ri: [[ https://github.com/llvm-mirror/clang/commit/526e627d2bd7e8cbf630526d315c90864898d9ff#diff… | |||||

Not Done ReplyTried implementing So, new options: - Either simply restore the original check, and add a comment there about the logic behind it
- Or, drop the check completely
- Or, move the
`CheckTautologicalComparison()`call before`DiagnoseOutOfRangeComparison()`And if`DiagnoseOutOfRangeComparison()`has already emitted diagnostic, return. Much like what`CheckTautologicalComparison()`already does.
So i think lebedev.ri: Tried implementing `3.`.
It won't work, because `DiagnoseOutOfRangeComparison()` needs the `{L… | |||||

8657 | 8701 | | |||

8658 | // Used for diagnostic printout. | 8702 | // Used for diagnostic printout. | ||

8659 | enum { | 8703 | enum { | ||

8660 | LiteralConstant = 0, | 8704 | LiteralConstant = 0, | ||

8661 | CXXBoolLiteralTrue, | 8705 | CXXBoolLiteralTrue, | ||

8662 | CXXBoolLiteralFalse | 8706 | CXXBoolLiteralFalse | ||

8663 | } LiteralOrBoolConstant = LiteralConstant; | 8707 | } LiteralOrBoolConstant = LiteralConstant; | ||

8664 | 8708 | | |||

8665 | if (!OtherIsBooleanType) { | 8709 | if (!OtherIsBooleanType) { | ||

8666 | QualType ConstantT = Constant->getType(); | 8710 | QualType ConstantT = Constant->getType(); | ||

8667 | QualType CommonT = E->getLHS()->getType(); | 8711 | QualType CommonT = E->getLHS()->getType(); | ||

8668 | 8712 | | |||

8669 | if (S.Context.hasSameUnqualifiedType(OtherT, ConstantT)) | 8713 | if (S.Context.hasSameUnqualifiedType(OtherT, ConstantT)) | ||

8670 | return; | 8714 | return false; | ||

8671 | assert((OtherT->isIntegerType() && ConstantT->isIntegerType()) && | 8715 | assert((OtherT->isIntegerType() && ConstantT->isIntegerType()) && | ||

8672 | "comparison with non-integer type"); | 8716 | "comparison with non-integer type"); | ||

8673 | 8717 | | |||

8674 | bool ConstantSigned = ConstantT->isSignedIntegerType(); | 8718 | bool ConstantSigned = ConstantT->isSignedIntegerType(); | ||

8675 | bool CommonSigned = CommonT->isSignedIntegerType(); | 8719 | bool CommonSigned = CommonT->isSignedIntegerType(); | ||

8676 | 8720 | | |||

8677 | bool EqualityOnly = false; | 8721 | bool EqualityOnly = false; | ||

8678 | 8722 | | |||

8679 | if (CommonSigned) { | 8723 | if (CommonSigned) { | ||

8680 | // The common type is signed, therefore no signed to unsigned conversion. | 8724 | // The common type is signed, therefore no signed to unsigned conversion. | ||

8681 | if (!OtherRange.NonNegative) { | 8725 | if (!OtherRange.NonNegative) { | ||

8682 | // Check that the constant is representable in type OtherT. | 8726 | // Check that the constant is representable in type OtherT. | ||

8683 | if (ConstantSigned) { | 8727 | if (ConstantSigned) { | ||

8684 | if (OtherWidth >= Value.getMinSignedBits()) | 8728 | if (OtherWidth >= Value.getMinSignedBits()) | ||

8685 | return; | 8729 | return false; | ||

8686 | } else { // !ConstantSigned | 8730 | } else { // !ConstantSigned | ||

8687 | if (OtherWidth >= Value.getActiveBits() + 1) | 8731 | if (OtherWidth >= Value.getActiveBits() + 1) | ||

8688 | return; | 8732 | return false; | ||

8689 | } | 8733 | } | ||

8690 | } else { // !OtherSigned | 8734 | } else { // !OtherSigned | ||

8691 | // Check that the constant is representable in type OtherT. | 8735 | // Check that the constant is representable in type OtherT. | ||

8692 | // Negative values are out of range. | 8736 | // Negative values are out of range. | ||

8693 | if (ConstantSigned) { | 8737 | if (ConstantSigned) { | ||

8694 | if (Value.isNonNegative() && OtherWidth >= Value.getActiveBits()) | 8738 | if (Value.isNonNegative() && OtherWidth >= Value.getActiveBits()) | ||

8695 | return; | 8739 | return false; | ||

8696 | } else { // !ConstantSigned | 8740 | } else { // !ConstantSigned | ||

8697 | if (OtherWidth >= Value.getActiveBits()) | 8741 | if (OtherWidth >= Value.getActiveBits()) | ||

8698 | return; | 8742 | return false; | ||

8699 | } | 8743 | } | ||

8700 | } | 8744 | } | ||

8701 | } else { // !CommonSigned | 8745 | } else { // !CommonSigned | ||

8702 | if (OtherRange.NonNegative) { | 8746 | if (OtherRange.NonNegative) { | ||

8703 | if (OtherWidth >= Value.getActiveBits()) | 8747 | if (OtherWidth >= Value.getActiveBits()) | ||

8704 | return; | 8748 | return false; | ||

8705 | } else { // OtherSigned | 8749 | } else { // OtherSigned | ||

8706 | assert(!ConstantSigned && | 8750 | assert(!ConstantSigned && | ||

8707 | "Two signed types converted to unsigned types."); | 8751 | "Two signed types converted to unsigned types."); | ||

8708 | // Check to see if the constant is representable in OtherT. | 8752 | // Check to see if the constant is representable in OtherT. | ||

8709 | if (OtherWidth > Value.getActiveBits()) | 8753 | if (OtherWidth > Value.getActiveBits()) | ||

8710 | return; | 8754 | return false; | ||

8711 | // Check to see if the constant is equivalent to a negative value | 8755 | // Check to see if the constant is equivalent to a negative value | ||

Done ReplyPlease try to reduce/factor out the duplication here. rsmith: Please try to reduce/factor out the duplication here. | |||||

8712 | // cast to CommonT. | 8756 | // cast to CommonT. | ||

8713 | if (S.Context.getIntWidth(ConstantT) == | 8757 | if (S.Context.getIntWidth(ConstantT) == | ||

8714 | S.Context.getIntWidth(CommonT) && | 8758 | S.Context.getIntWidth(CommonT) && | ||

8715 | Value.isNegative() && Value.getMinSignedBits() <= OtherWidth) | 8759 | Value.isNegative() && Value.getMinSignedBits() <= OtherWidth) | ||

8716 | return; | 8760 | return false; | ||

8717 | // The constant value rests between values that OtherT can represent | 8761 | // The constant value rests between values that OtherT can represent | ||

8718 | // after conversion. Relational comparison still works, but equality | 8762 | // after conversion. Relational comparison still works, but equality | ||

8719 | // comparisons will be tautological. | 8763 | // comparisons will be tautological. | ||

8720 | EqualityOnly = true; | 8764 | EqualityOnly = true; | ||

8721 | } | 8765 | } | ||

8722 | } | 8766 | } | ||

8723 | 8767 | | |||

8724 | bool PositiveConstant = !ConstantSigned || Value.isNonNegative(); | 8768 | bool PositiveConstant = !ConstantSigned || Value.isNonNegative(); | ||

8725 | 8769 | | |||

8726 | if (op == BO_EQ || op == BO_NE) { | 8770 | if (op == BO_EQ || op == BO_NE) { | ||

8727 | IsTrue = op == BO_NE; | 8771 | IsTrue = op == BO_NE; | ||

8728 | } else if (EqualityOnly) { | 8772 | } else if (EqualityOnly) { | ||

8729 | return; | 8773 | return false; | ||

8730 | } else if (RhsConstant) { | 8774 | } else if (RhsConstant) { | ||

8731 | if (op == BO_GT || op == BO_GE) | 8775 | if (op == BO_GT || op == BO_GE) | ||

8732 | IsTrue = !PositiveConstant; | 8776 | IsTrue = !PositiveConstant; | ||

8733 | else // op == BO_LT || op == BO_LE | 8777 | else // op == BO_LT || op == BO_LE | ||

8734 | IsTrue = PositiveConstant; | 8778 | IsTrue = PositiveConstant; | ||

8735 | } else { | 8779 | } else { | ||

8736 | if (op == BO_LT || op == BO_LE) | 8780 | if (op == BO_LT || op == BO_LE) | ||

8737 | IsTrue = !PositiveConstant; | 8781 | IsTrue = !PositiveConstant; | ||

▲ Show 20 Lines • Show All 71 Lines • ▼ Show 20 Line(s) | 8851 | default: | |||

8809 | break; | 8853 | break; | ||

8810 | } | 8854 | } | ||

8811 | 8855 | | |||

8812 | if (CmpRes == AFals) { | 8856 | if (CmpRes == AFals) { | ||

8813 | IsTrue = false; | 8857 | IsTrue = false; | ||

8814 | } else if (CmpRes == ATrue) { | 8858 | } else if (CmpRes == ATrue) { | ||

8815 | IsTrue = true; | 8859 | IsTrue = true; | ||

8816 | } else { | 8860 | } else { | ||

8817 | return; | 8861 | return false; | ||

8818 | } | 8862 | } | ||

8819 | } | 8863 | } | ||

8820 | 8864 | | |||

8821 | // If this is a comparison to an enum constant, include that | 8865 | // If this is a comparison to an enum constant, include that | ||

8822 | // constant in the diagnostic. | 8866 | // constant in the diagnostic. | ||

8823 | const EnumConstantDecl *ED = nullptr; | 8867 | const EnumConstantDecl *ED = nullptr; | ||

8824 | if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Constant)) | 8868 | if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Constant)) | ||

8825 | ED = dyn_cast<EnumConstantDecl>(DR->getDecl()); | 8869 | ED = dyn_cast<EnumConstantDecl>(DR->getDecl()); | ||

8826 | 8870 | | |||

8827 | SmallString<64> PrettySourceValue; | 8871 | SmallString<64> PrettySourceValue; | ||

8828 | llvm::raw_svector_ostream OS(PrettySourceValue); | 8872 | llvm::raw_svector_ostream OS(PrettySourceValue); | ||

8829 | if (ED) | 8873 | if (ED) | ||

8830 | OS << '\'' << *ED << "' (" << Value << ")"; | 8874 | OS << '\'' << *ED << "' (" << Value << ")"; | ||

8831 | else | 8875 | else | ||

8832 | OS << Value; | 8876 | OS << Value; | ||

8833 | 8877 | | |||

8834 | S.DiagRuntimeBehavior( | 8878 | S.DiagRuntimeBehavior( | ||

8835 | E->getOperatorLoc(), E, | 8879 | E->getOperatorLoc(), E, | ||

8836 | S.PDiag(diag::warn_out_of_range_compare) | 8880 | S.PDiag(diag::warn_out_of_range_compare) | ||

8837 | << OS.str() << LiteralOrBoolConstant | 8881 | << OS.str() << LiteralOrBoolConstant | ||

8838 | << OtherT << (OtherIsBooleanType && !OtherT->isBooleanType()) << IsTrue | 8882 | << OtherT << (OtherIsBooleanType && !OtherT->isBooleanType()) << IsTrue | ||

8839 | << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); | 8883 | << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); | ||

8884 | | ||||

8885 | return true; | ||||

8840 | } | 8886 | } | ||

8841 | 8887 | | |||

8842 | /// Analyze the operands of the given comparison. Implements the | 8888 | /// Analyze the operands of the given comparison. Implements the | ||

8843 | /// fallback case from AnalyzeComparison. | 8889 | /// fallback case from AnalyzeComparison. | ||

8844 | void AnalyzeImpConvsInComparison(Sema &S, BinaryOperator *E) { | 8890 | void AnalyzeImpConvsInComparison(Sema &S, BinaryOperator *E) { | ||

8845 | AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc()); | 8891 | AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc()); | ||

8846 | AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc()); | 8892 | AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc()); | ||

8847 | } | 8893 | } | ||

Show All 9 Lines | 8898 | void AnalyzeComparison(Sema &S, BinaryOperator *E) { | |||

8857 | // the same type. | 8903 | // the same type. | ||

8858 | if (!S.Context.hasSameUnqualifiedType(T, E->getRHS()->getType())) | 8904 | if (!S.Context.hasSameUnqualifiedType(T, E->getRHS()->getType())) | ||

8859 | return AnalyzeImpConvsInComparison(S, E); | 8905 | return AnalyzeImpConvsInComparison(S, E); | ||

8860 | 8906 | | |||

8861 | // Don't analyze value-dependent comparisons directly. | 8907 | // Don't analyze value-dependent comparisons directly. | ||

8862 | if (E->isValueDependent()) | 8908 | if (E->isValueDependent()) | ||

8863 | return AnalyzeImpConvsInComparison(S, E); | 8909 | return AnalyzeImpConvsInComparison(S, E); | ||

8864 | 8910 | | |||

8865 | Expr *LHS = E->getLHS()->IgnoreParenImpCasts(); | 8911 | Expr *LHS = E->getLHS(); | ||

8866 | Expr *RHS = E->getRHS()->IgnoreParenImpCasts(); | 8912 | Expr *RHS = E->getRHS(); | ||

8867 | | ||||

8868 | bool IsComparisonConstant = false; | | |||

8869 | 8913 | | |||

8870 | // Check whether an integer constant comparison results in a value | | |||

8871 | // of 'true' or 'false'. | | |||

8872 | if (T->isIntegralType(S.Context)) { | 8914 | if (T->isIntegralType(S.Context)) { | ||

8873 | llvm::APSInt RHSValue; | 8915 | llvm::APSInt RHSValue; | ||

8874 | bool IsRHSIntegralLiteral = | | |||

8875 | RHS->isIntegerConstantExpr(RHSValue, S.Context); | | |||

8876 | llvm::APSInt LHSValue; | 8916 | llvm::APSInt LHSValue; | ||

8877 | bool IsLHSIntegralLiteral = | 8917 | | ||

8878 | LHS->isIntegerConstantExpr(LHSValue, S.Context); | 8918 | bool IsRHSIntegralLiteral = RHS->isIntegerConstantExpr(RHSValue, S.Context); | ||

8879 | if (IsRHSIntegralLiteral && !IsLHSIntegralLiteral) | 8919 | bool IsLHSIntegralLiteral = LHS->isIntegerConstantExpr(LHSValue, S.Context); | ||

8880 | DiagnoseOutOfRangeComparison(S, E, RHS, LHS, RHSValue, true); | 8920 | | ||

8881 | else if (!IsRHSIntegralLiteral && IsLHSIntegralLiteral) | 8921 | // We don't care about expressions whose result is a constant. | ||

8882 | DiagnoseOutOfRangeComparison(S, E, LHS, RHS, LHSValue, false); | 8922 | if (IsRHSIntegralLiteral && IsLHSIntegralLiteral) | ||

8883 | else | 8923 | return AnalyzeImpConvsInComparison(S, E); | ||

Done ReplyI don't think this variable pulls its weight any more, especially given the adjacent comment. Inline its initializer into the rsmith: I don't think this variable pulls its weight any more, especially given the adjacent comment. | |||||

8884 | IsComparisonConstant = | 8924 | | ||

Done ReplyPass rsmith: Pass `LHSValue` and `RHSValue` in here rather than recomputing them. | |||||

Not Done ReplyWe can even go one step further, and pass the lebedev.ri: We can even go one step further, and pass the `bool IsConstLiteralOnRHS` | |||||

8885 | (IsRHSIntegralLiteral && IsLHSIntegralLiteral); | 8925 | // We only care about expressions where just one side is literal | ||

8886 | } else if (!T->hasUnsignedIntegerRepresentation()) | 8926 | if (IsRHSIntegralLiteral ^ IsLHSIntegralLiteral) { | ||

Not Done ReplyIt would probably be more obvious to use rsmith: It would probably be more obvious to use `||` here, since you already returned in the case… | |||||

Not Done ReplyAnd what if neither of them is constant? lebedev.ri: And what if neither of them is constant?
I *think* we could reach this point in that case.
At… | |||||

8887 | IsComparisonConstant = E->isIntegerConstantExpr(S.Context); | 8927 | // Is the constant on the RHS or LHS? | ||

Done ReplyIt seems suboptimal to evaluate both sides of the comparison and then evaluate the entire comparison again in this case. Presumably the point is to catch comparisons whose operands are not of integral type (eg, floating point), but we could get that benefit and also catch more integral cases by switching from I'm fine with a cleanup to avoid the repeated evaluation here being deferred to a later patch. rsmith: It seems suboptimal to evaluate both sides of the comparison and then evaluate the entire… | |||||

Not Done ReplyIf we look at this code closely, if lebedev.ri: If we look at this code closely, if `hasUnsignedIntegerRepresentation() == false`, we always do… | |||||

Not Done ReplyIt looks like the old behavior was to suppress the - the comparison has a constant value, and
- the types being compared are not integral types, and
- the types being compared do not have an unsigned integer representation
However, rsmith: It looks like the old behavior was to suppress the `CheckTautologicalComparisonWithZero`… | |||||

8888 | 8928 | const bool RhsConstant = IsRHSIntegralLiteral; | |||

8889 | // We don't care about value-dependent expressions or expressions | 8929 | Expr *Const = RhsConstant ? RHS : LHS; | ||

8890 | // whose result is a constant. | 8930 | Expr *Other = RhsConstant ? LHS : RHS; | ||

8891 | if (IsComparisonConstant) | 8931 | const llvm::APSInt &Value = RhsConstant ? RHSValue : LHSValue; | ||

8932 | | ||||

8933 | // Check whether an integer constant comparison results in a value | ||||

8934 | // of 'true' or 'false'. | ||||

8935 | | ||||

8936 | if (CheckTautologicalComparison(S, E, Const, Other, Value, RhsConstant)) | ||||

8892 | return AnalyzeImpConvsInComparison(S, E); | 8937 | return AnalyzeImpConvsInComparison(S, E); | ||

8893 | 8938 | | |||

8894 | // If this is a tautological comparison, suppress -Wsign-compare. | 8939 | if (DiagnoseOutOfRangeComparison(S, E, Const, Other, Value, RhsConstant)) | ||

8895 | if (CheckTautologicalComparisonWithZero(S, E)) | | |||

8896 | return AnalyzeImpConvsInComparison(S, E); | 8940 | return AnalyzeImpConvsInComparison(S, E); | ||

8941 | } | ||||

8942 | } | ||||

8897 | 8943 | | |||

8944 | if (!T->hasUnsignedIntegerRepresentation()) { | ||||

8898 | // We don't do anything special if this isn't an unsigned integral | 8945 | // We don't do anything special if this isn't an unsigned integral | ||

8899 | // comparison: we're only interested in integral comparisons, and | 8946 | // comparison: we're only interested in integral comparisons, and | ||

8900 | // signed comparisons only happen in cases we don't care to warn about. | 8947 | // signed comparisons only happen in cases we don't care to warn about. | ||

8901 | if (!T->hasUnsignedIntegerRepresentation()) | | |||

8902 | return AnalyzeImpConvsInComparison(S, E); | 8948 | return AnalyzeImpConvsInComparison(S, E); | ||

8949 | } | ||||

8950 | | ||||

8951 | LHS = LHS->IgnoreParenImpCasts(); | ||||

8952 | RHS = RHS->IgnoreParenImpCasts(); | ||||

8903 | 8953 | | |||

8904 | // Check to see if one of the (unmodified) operands is of different | 8954 | // Check to see if one of the (unmodified) operands is of different | ||

8905 | // signedness. | 8955 | // signedness. | ||

8906 | Expr *signedOperand, *unsignedOperand; | 8956 | Expr *signedOperand, *unsignedOperand; | ||

8907 | if (LHS->getType()->hasSignedIntegerRepresentation()) { | 8957 | if (LHS->getType()->hasSignedIntegerRepresentation()) { | ||

8908 | assert(!RHS->getType()->hasSignedIntegerRepresentation() && | 8958 | assert(!RHS->getType()->hasSignedIntegerRepresentation() && | ||

8909 | "unsigned comparison between two signed integer expressions?"); | 8959 | "unsigned comparison between two signed integer expressions?"); | ||

8910 | signedOperand = LHS; | 8960 | signedOperand = LHS; | ||

▲ Show 20 Lines • Show All 3475 Lines • Show Last 20 Lines |

Use

///for documentation comments.