[clang-tidy] bugprone-implicit-widening ignores unsigned consts

clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp

@@ -88,13 +88,15 @@ void ImplicitWideningOfMultiplicationResultCheck::handleImplicitCastExpr(
 
   // Is the expression a compile-time constexpr that we know can fit in the
   // source type?
-  if (IgnoreConstantIntExpr && ETy->isIntegerType() &&
-      !ETy->isUnsignedIntegerType()) {
-    if (const auto ConstExprResult = E->getIntegerConstantExpr(*Context)) {
+  if (IgnoreConstantIntExpr && ETy->isIntegerType()) {
+    if (const auto ConstExprResult =
+            E->getIntegerConstantExpr(*Context, nullptr, true)) {
       const auto TypeSize = Context->getTypeSize(ETy);
+      const auto Unsigned = ETy->isUnsignedIntegerType();
+
       llvm::APSInt WidenedResult = ConstExprResult->extOrTrunc(TypeSize);
-      if (WidenedResult <= llvm::APSInt::getMaxValue(TypeSize, false) &&
-          WidenedResult >= llvm::APSInt::getMinValue(TypeSize, false))
+      if (WidenedResult <= llvm::APSInt::getMaxValue(TypeSize, Unsigned) &&
+          WidenedResult >= llvm::APSInt::getMinValue(TypeSize, Unsigned))
         return;
     }
   }

clang-tools-extra/docs/ReleaseNotes.rst

@@ -121,6 +121,11 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/forwarding-reference-overload>` check by fixing
   a crash when determining if an ``enable_if[_t]`` was found.
 
+- Improved :doc:`bugprone-implicit-widening-of-multiplication-result
+  <clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result>` check
+  by allowing the `IgnoreConstantIntExpr` option to apply to both signed and
+  unsigned integer types.
+
 - Improved :doc:`bugprone-sizeof-expression
   <clang-tidy/checks/bugprone/sizeof-expression>` check to find suspicious
   usages of ``sizeof()``, ``alignof()``, and ``offsetof()`` when adding or

clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst

@@ -37,20 +37,19 @@ Options
 .. option:: UseCXXStaticCastsInCppSources
 
    When suggesting fix-its for C++ code, should C++-style ``static_cast<>()``'s
-   be suggested, or C-style casts. Defaults to ``true``.
+   be suggested, or C-style casts. Defaults to `true`.
 
 .. option:: UseCXXHeadersInCppSources
 
    When suggesting to include the appropriate header in C++ code,
    should ``<cstddef>`` header be suggested, or ``<stddef.h>``.
-   Defaults to ``true``.
+   Defaults to `true`.
 
 .. option:: IgnoreConstantIntExpr
 
    If the multiplication operands are compile-time constants (like literals or
    are ``constexpr``) and fit within the source expression type, do not emit a
-   diagnostic or suggested fix.  Only considers expressions where the source
-   expression is a signed integer type.  Defaults to ``false``.
+   diagnostic or suggested fix.  Defaults to `false`.
 
 Examples:
 

clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp

@@ -40,6 +40,11 @@ long t5() {
   return 1 * min_int;
 }
 
+long t6() {
+  const unsigned int a = 4294967295u; // unsigned int max
+  return a * 1u;
+}
+
 unsigned long n0() {
   const int a = 1073741824; // 1/2 of int32_t max
   const int b = 3;
@@ -50,7 +55,11 @@ unsigned long n0() {
   // CHECK-MESSAGES: :[[@LINE-4]]:10: note: perform multiplication in a wider type
 }
 
-double n1() {
-  const long a = 100000000;
-  return a * 400;
+long n1() {
+  const unsigned int a = 4294967295u; // unsigned int max
+  return a * 3u;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'long' of a multiplication performed in type 'unsigned int'
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+  // CHECK-MESSAGES:                  static_cast<long>( )
+  // CHECK-MESSAGES: :[[@LINE-4]]:10: note: perform multiplication in a wider type
 }

clang/include/clang/AST/Expr.h

@@ -551,11 +551,15 @@ class Expr : public ValueStmt {
   /// and fill in Loc (if specified) with the location of the invalid
   /// expression.
   ///
+  /// If CheckUnsignedOverflow is true, the i-c-e is of an unsigned type, and
+  /// the i-c-e result cannot fit into the expression type without overflowing,
+  /// std::nullopt is returned.
+  ///
   /// Note: This does not perform the implicit conversions required by C++11
   /// [expr.const]p5.
   std::optional<llvm::APSInt>
-  getIntegerConstantExpr(const ASTContext &Ctx,
-                         SourceLocation *Loc = nullptr) const;
+  getIntegerConstantExpr(const ASTContext &Ctx, SourceLocation *Loc = nullptr,
+                         bool CheckUnsignedOverflow = false) const;
   bool isIntegerConstantExpr(const ASTContext &Ctx,
                              SourceLocation *Loc = nullptr) const;
 
@@ -569,7 +573,8 @@ class Expr : public ValueStmt {
   /// Note: This does not perform the implicit conversions required by C++11
   /// [expr.const]p5.
   bool isCXX11ConstantExpr(const ASTContext &Ctx, APValue *Result = nullptr,
-                           SourceLocation *Loc = nullptr) const;
+                           SourceLocation *Loc = nullptr,
+                           bool CheckUnsignedOverflow = false) const;
 
   /// isPotentialConstantExpr - Return true if this function's definition
   /// might be usable in a constant expression in C++11, if it were marked

clang/lib/AST/ExprConstant.cpp

@@ -980,6 +980,12 @@ namespace {
     /// is set; this is used when evaluating ICEs in C.
     bool CheckingForUndefinedBehavior = false;
 
+    /// Whether we're checking for an expression that causing the unsigned
+    /// integer expression type to overflow and wrap-around back to 0 when
+    /// evaluating an ICE.  If set to true, the ICE evaluation will fail as
+    /// though the expression could not be calculated at compile time
+    bool CheckingForUnsignedIntegerOverflow = false;
+
     enum EvaluationMode {
       /// Evaluate as a constant expression. Stop if we find that the expression
       /// is not a constant expression.
@@ -2795,24 +2801,28 @@ static bool truncateBitfieldValue(EvalInfo &Info, const Expr *E,
 
 /// Perform the given integer operation, which is known to need at most BitWidth
 /// bits, and check for overflow in the original type (if that type was not an
-/// unsigned type).
+/// unsigned type or EvalInfo.CheckingForUnsignedIntegerOverflow is true).
 template<typename Operation>
 static bool CheckedIntArithmetic(EvalInfo &Info, const Expr *E,
                                  const APSInt &LHS, const APSInt &RHS,
                                  unsigned BitWidth, Operation Op,
                                  APSInt &Result) {
-  if (LHS.isUnsigned()) {
+  if (LHS.isUnsigned() && !Info.CheckingForUnsignedIntegerOverflow) {
     Result = Op(LHS, RHS);
     return true;
   }
 
-  APSInt Value(Op(LHS.extend(BitWidth), RHS.extend(BitWidth)), false);
+  APSInt Value(Op(LHS.extend(BitWidth), RHS.extend(BitWidth)),
+               LHS.isUnsigned());
   Result = Value.trunc(LHS.getBitWidth());
   if (Result.extend(BitWidth) != Value) {
+    if (Result.isUnsigned())
+      return false;
     if (Info.checkingForUndefinedBehavior())
       Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
                                        diag::warn_integer_constant_overflow)
-          << toString(Result, 10, Result.isSigned(), /*formatAsCLiteral=*/false,
+          << toString(Result, 10, Result.isSigned(),
+                      /*formatAsCLiteral=*/false,
                       /*UpperCase=*/true, /*InsertSeparators=*/true)
           << E->getType() << E->getSourceRange();
     return HandleOverflow(Info, E, Value, E->getType());
@@ -16980,17 +16990,16 @@ static ICEDiag CheckICE(const Expr* E, const ASTContext &Ctx) {
 }
 
 /// Evaluate an expression as a C++11 integral constant expression.
-static bool EvaluateCPlusPlus11IntegralConstantExpr(const ASTContext &Ctx,
-                                                    const Expr *E,
-                                                    llvm::APSInt *Value,
-                                                    SourceLocation *Loc) {
+static bool EvaluateCPlusPlus11IntegralConstantExpr(
+    const ASTContext &Ctx, const Expr *E, llvm::APSInt *Value,
+    SourceLocation *Loc, bool CheckUnsignedOverflow) {
   if (!E->getType()->isIntegralOrUnscopedEnumerationType()) {
     if (Loc) *Loc = E->getExprLoc();
     return false;
   }
 
   APValue Result;
-  if (!E->isCXX11ConstantExpr(Ctx, &Result, Loc))
+  if (!E->isCXX11ConstantExpr(Ctx, &Result, Loc, CheckUnsignedOverflow))
     return false;
 
   if (!Result.isInt()) {
@@ -17010,7 +17019,8 @@ bool Expr::isIntegerConstantExpr(const ASTContext &Ctx,
   ExprTimeTraceScope TimeScope(this, Ctx, "isIntegerConstantExpr");
 
   if (Ctx.getLangOpts().CPlusPlus11)
-    return EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, nullptr, Loc);
+    return EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, nullptr, Loc,
+                                                   false);
 
   ICEDiag D = CheckICE(this, Ctx);
   if (D.Kind != IK_ICE) {
@@ -17021,7 +17031,8 @@ bool Expr::isIntegerConstantExpr(const ASTContext &Ctx,
 }
 
 std::optional<llvm::APSInt>
-Expr::getIntegerConstantExpr(const ASTContext &Ctx, SourceLocation *Loc) const {
+Expr::getIntegerConstantExpr(const ASTContext &Ctx, SourceLocation *Loc,
+                             bool CheckUnsignedOverflow) const {
   if (isValueDependent()) {
     // Expression evaluator can't succeed on a dependent expression.
     return std::nullopt;
@@ -17030,7 +17041,8 @@ Expr::getIntegerConstantExpr(const ASTContext &Ctx, SourceLocation *Loc) const {
   APSInt Value;
 
   if (Ctx.getLangOpts().CPlusPlus11) {
-    if (EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, &Value, Loc))
+    if (EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, &Value, Loc,
+                                                CheckUnsignedOverflow))
       return Value;
     return std::nullopt;
   }
@@ -17061,7 +17073,8 @@ bool Expr::isCXX98IntegralConstantExpr(const ASTContext &Ctx) const {
 }
 
 bool Expr::isCXX11ConstantExpr(const ASTContext &Ctx, APValue *Result,
-                               SourceLocation *Loc) const {
+                               SourceLocation *Loc,
+                               bool CheckUnsignedOverflow) const {
   assert(!isValueDependent() &&
          "Expression evaluator can't be called on a dependent expression.");
 
@@ -17074,6 +17087,7 @@ bool Expr::isCXX11ConstantExpr(const ASTContext &Ctx, APValue *Result,
   SmallVector<PartialDiagnosticAt, 8> Diags;
   Status.Diag = &Diags;
   EvalInfo Info(Ctx, Status, EvalInfo::EM_ConstantExpression);
+  Info.CheckingForUnsignedIntegerOverflow = CheckUnsignedOverflow;
 
   APValue Scratch;
   bool IsConstExpr =

clang/unittests/AST/ASTExprTest.cpp

@@ -108,3 +108,39 @@ TEST(ASTExpr, InitListIsConstantInitialized) {
   (void)FooInit->updateInit(Ctx, 2, Ref);
   EXPECT_FALSE(FooInit->isConstantInitializer(Ctx, false));
 }
+
+TEST(ASTExpr, NoUnsignedOverflowICE) {
+  auto AST = buildASTFromCode(R"cpp(
+    unsigned u = 1u * 8u;
+  )cpp");
+  ASTContext &Ctx = AST->getASTContext();
+  const VarDecl *Var = getVariableNode(AST.get(), "u");
+
+  const auto ICE = Var->getInit()->getIntegerConstantExpr(Ctx);
+  EXPECT_TRUE(ICE.has_value());
+  EXPECT_TRUE(ICE->isUnsigned());
+  EXPECT_EQ(8u, *ICE);
+
+  const auto ICEOverflowCheck =
+      Var->getInit()->getIntegerConstantExpr(Ctx, nullptr, true);
+  EXPECT_TRUE(ICEOverflowCheck.has_value());
+  EXPECT_TRUE(ICEOverflowCheck->isUnsigned());
+  EXPECT_EQ(8u, *ICEOverflowCheck);
+}
+
+TEST(ASTExpr, CheckForUnsignedOverflowICE) {
+  auto AST = buildASTFromCode(R"cpp(
+    unsigned u = 1'000'000'000u * 8u;
+  )cpp");
+  ASTContext &Ctx = AST->getASTContext();
+  const VarDecl *Var = getVariableNode(AST.get(), "u");
+
+  const auto ICE = Var->getInit()->getIntegerConstantExpr(Ctx);
+  EXPECT_TRUE(ICE.has_value());
+  EXPECT_TRUE(ICE->isUnsigned());
+  EXPECT_EQ(3'705'032'704u, *ICE);
+
+  const auto ICEOverflowCheck =
+      Var->getInit()->getIntegerConstantExpr(Ctx, nullptr, true);
+  EXPECT_FALSE(ICEOverflowCheck.has_value());
+}