[clang-tidy] [bugprone-implicit-widening-of-multiplication-result] Improved check to ignore false positives with integer literals.

[felix642]

Summary

The following code is safe and should not trigger the warning
constexpr std::size_t k1Mb = 1024 * 1024;

Fixes #64732

[felix642]

Fixed format

[PiotrZSL]

I'm not sure if this is right fix, example:

const std::size_t k1Tb = 1024 * 1024 * 1024 * 1024;

This is detected by -Winteger-overflow, but this:

const  std::size_t k1Pb =  1024U * 1024U * 1024U * 1024U * 1024U;

is not detect by anything except this check, even that it overflow to 0.
Funny thing that if we change those unsigned to signed, then its detected by -Winteger-overflow

For me we shoudn't silent those issues, if someone want they they can always put nolint or explicit cast, we could consider adding some basic calculations just to verify if there will be no overflow, but still proper way would be to provide warning that would say, hey, write this as: "1024LU * 1024LU"

[PiotrZSL]

In short I'm against simply ignoring literal calculations because this check is only thing that currently detect this issue: https://github.com/llvm/llvm-project/issues/64828

[felix642]

Hi @PiotrZSL thank you for taking the time to look at this revision.

I agree with you we should not silence a warning if no other tool can diagnose the issue. I'm guessing that -Winteger-overflow does no trigger any warning on unsigned "overflow" since behavior is well defined in the standard :

46. This implies that unsigned arithmetic does not overflow because a result that cannot be represented by the resulting unsigned integer type is reduced modulo the number that is one greater than the largest value that can be represented by the resulting unsigned integer type.

But on the other hand I have to agree with DenisYaroshevskiy that it is tedious to add a cast to every multiplication of integer literals on a large codebase. Especially when we know that those values do not overflow.

Maybe we should add some basic calculations when the operation is composed of integer literals, like you previously mentioned, to check if the operation actually overflows and print this warning if it does?
In that case we could also improve the fix-it and suggest to add LU or U instead of static_cast.

[PiotrZSL]

Yes doing some basic calculations would be probably the best, but for that we would need to implement some "calculator" or find some exist implementation some're in clang.
Even if it would support only basic operations like *

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

static const Expr *getLHSOfMulBinOp(const Expr *E) {
   // FIXME: shall we skip brackets/casts/etc?
   const auto *BO = dyn_cast<BinaryOperator>(E);
   if (!BO || BO->getOpcode() != BO_Mul)
     // FIXME: what about:  long r = int(x) + (int(y) * int(z));  ?
     return nullptr;
   return BO->getLHS()->IgnoreParens();
 }
 
+static bool hasIntegerLiteralOperators(const Expr *E) {
+  const auto *BO = dyn_cast<BinaryOperator>(E);
+  return isa<IntegerLiteral>(BO->getLHS()->IgnoreParenImpCasts()) &&
+         isa<IntegerLiteral>(BO->getRHS()->IgnoreParenImpCasts());
+}
+
 ImplicitWideningOfMultiplicationResultCheck::
     ImplicitWideningOfMultiplicationResultCheck(StringRef Name,
                                                 ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       UseCXXStaticCastsInCppSources(
           Options.get("UseCXXStaticCastsInCppSources", true)),
       UseCXXHeadersInCppSources(Options.get("UseCXXHeadersInCppSources", true)),
       IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
if (TgtWidth <= SrcWidth)
     return;
 
   // Does the index expression look like it might be unintentionally computed
   // in a narrower-than-wanted type?
   const Expr *LHS = getLHSOfMulBinOp(E);
   if (!LHS)
     return;
 
+  // Widening multiplication on Integer Literals.
+  if (hasIntegerLiteralOperators(E))
+    return;
+
   // Ok, looks like we should diagnose this.
   diag(E->getBeginLoc(), "performing an implicit widening conversion to type "
                          "%0 of a multiplication performed in type %1")
       << Ty << E->getType();
 
   {
     auto Diag = diag(E->getBeginLoc(),
                      "make conversion explicit to silence this warning",

clang-tools-extra/docs/ReleaseNotes.rst

 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Fixed bug in :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone/reserved-identifier>`, so that it does not warn
   on macros starting with underscore and lowercase letter.
 
+- Improved :doc:`bugprone-implicit-widening-of-multiplication-result
+  <clang-tidy/checks/bugprone/bugprone-implicit-widening-of-multiplication-result>`
+  check to ignore false-positives with integer literals.
+
 - Improved :doc:`bugprone-lambda-function-name
   <clang-tidy/checks/bugprone/lambda-function-name>` check by adding option
   `IgnoreMacros` to ignore warnings in macros.
 
 - Improved :doc:`cppcoreguidelines-avoid-non-const-global-variables
   <clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables>` check
   to ignore ``static`` variables declared within the scope of
   ``class``/``struct``.

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

 }
 long n13(int a, long b) {
   return a * b;
 }
 
 long n14(int a, int b, int c) {
   return a + b * c;
 }
+
 long n15(int a, int b, int c) {
   return a * b + c;
 }
 
+unsigned long n16()
+{
+  return (1024u) * 1024;
+}
+
+long n17(int a) {
+  return a + 1024 * 1024;
+}
+
+long n18(int a)
+{
+  return (a * 1024);
+  // CHECK-NOTES-ALL: :[[@LINE-1]]:11: warning: performing an implicit widening conversion to type 'long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-ALL: :[[@LINE-2]]:11: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-ALL: :[[@LINE-3]]:11: note: perform multiplication in a wider type
+}
+
 #ifdef __cplusplus
 template <typename T1, typename T2>
 T2 template_test(T1 a, T1 b) {
   return a * b;
 }
 long template_test_instantiation(int a, int b) {
   return template_test<int, long>(a, b);
 }
 #endif