[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
cwarner-8702 wrote: > Actually it's oposite. Simply when -Winteger-overflow is enabled, then > IgnoreConstantIntExpr can be set to true, to avoid duplicated warnings. For > me IgnoreConstantIntExpr should even by ON by default. Sorry, I'm still struggling to see the connection. Maybe I'm getting too caught up on the fact one's for the compiler, the other is for clang-tidy, and they don't affect one another. If both are enabled, you will still get duplicated warnings if there is in fact an overflow problem. The change here is that if there isn't a real overflow problem, `IgnoreConstantIntExpr` will be silent just like `-Winteger-overflow` will. So it just brings them into alignment. > For me IgnoreConstantIntExpr should even by ON by default. I was trying to minimize impact, but I'm certainly open to it. :-) https://github.com/llvm/llvm-project/pull/98352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
@@ -50,7 +50,7 @@ Options 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. + expression is a signed integer type. Defaults to ``false``. cwarner-8702 wrote: The other options all use double ticks for the default value; I'd think it should be the same for this one https://github.com/llvm/llvm-project/pull/98352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
PiotrZSL wrote: You could also mention in documentation of option that those issues are detected by compiler warning: `-Winteger-overflow` https://github.com/llvm/llvm-project/pull/98352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
@@ -45,6 +45,12 @@ Options should header be suggested, or . 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. PiotrZSL wrote: Add info about default value at the end https://github.com/llvm/llvm-project/pull/98352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
https://github.com/PiotrZSL edited https://github.com/llvm/llvm-project/pull/98352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
@@ -84,6 +86,19 @@ void ImplicitWideningOfMultiplicationResultCheck::handleImplicitCastExpr( if (TgtWidth <= SrcWidth) return; + // Is the expression a compile-time constexpr that we know can fit in the + // source type? + if (IgnoreConstantIntExpr && ETy->isIntegerType() && PiotrZSL wrote: why limiting this only to Int, why not supporting unsigned int also ? https://github.com/llvm/llvm-project/pull/98352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
@@ -0,0 +1,81 @@ +// RUN: %check_clang_tidy -check-suffixes=ALL,NI %s bugprone-implicit-widening-of-multiplication-result %t -- -- -target x86_64-unknown-unknown -x c++ PiotrZSL wrote: i have feeling that this test isn't needed here at all, and second would be sufficient, basic scenarios could be covered in implicit-widening-of-multiplication-result.cpp file. https://github.com/llvm/llvm-project/pull/98352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy -check-suffixes=ALL,NI %s
bugprone-implicit-widening-of-multiplication-result %t -- -- -target
x86_64-unknown-unknown -x c++
+// RUN: %check_clang_tidy -check-suffixes=ALL %s
bugprone-implicit-widening-of-multiplication-result %t -- \
+// RUN: -config='{CheckOptions: { \
+// RUN:
bugprone-implicit-widening-of-multiplication-result.IgnoreConstantIntExpr: true
\
+// RUN: }}' -- -target x86_64-unknown-unknown -x c++
+
+long t0() {
+ return 1 * 4;
+ // CHECK-NOTES-NI: :[[@LINE-1]]:10: warning: performing an implicit widening
conversion to type 'long' of a multiplication performed in type 'int'
PiotrZSL wrote:
Generic: use CHECK-MESSAGES instead of CHECK-NOTES if you can
https://github.com/llvm/llvm-project/pull/98352
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
https://github.com/PiotrZSL commented: Missing release notes entry about added new config options for check. https://github.com/llvm/llvm-project/pull/98352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
cwarner-8702 wrote: @AaronBallman @njames93 Could you review? https://github.com/llvm/llvm-project/pull/98352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
llvmbot wrote:
@llvm/pr-subscribers-clang-tidy
Author: Chris Warner (cwarner-8702)
Changes
Add an option to the `bugprone-implicit-widening-of-multiplication-result`
clang-tidy checker to suppress warnings when the expression is made up of all
compile-time constants (literals, `constexpr` values or results, etc.) and the
result of the multiplication is guaranteed to fit in both the source and
destination types.
This currently only works for signed integer types:
* For unsigned integers, the well-defined rollover behavior on overflow
prevents the checker from detecting that the expression does not actually fit
in the source expr type, and would produce false negatives. I have a
somewhat-tacky stab at addressing this I'd like to submit as a follow-on PR
* For floating-point types, there's probably a little additional work to be
done to `Expr` to calculate the result at compile time
Even still, this seems like a helpful addition just for signed types, and
additional types can be added.
---
Full diff: https://github.com/llvm/llvm-project/pull/98352.diff
4 Files Affected:
- (modified)
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
(+15)
- (modified)
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
(+1)
- (modified)
clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
(+6)
- (added)
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp
(+81)
``diff
diff --git
a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
index f99beac668ce7..46bf20e34ce04 100644
---
a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
+++
b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
@@ -42,6 +42,7 @@ ImplicitWideningOfMultiplicationResultCheck::
UseCXXStaticCastsInCppSources(
Options.get("UseCXXStaticCastsInCppSources", true)),
UseCXXHeadersInCppSources(Options.get("UseCXXHeadersInCppSources",
true)),
+ IgnoreConstantIntExpr(Options.get("IgnoreConstantIntExpr", false)),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()) {}
@@ -56,6 +57,7 @@ void
ImplicitWideningOfMultiplicationResultCheck::storeOptions(
Options.store(Opts, "UseCXXStaticCastsInCppSources",
UseCXXStaticCastsInCppSources);
Options.store(Opts, "UseCXXHeadersInCppSources", UseCXXHeadersInCppSources);
+ Options.store(Opts, "IgnoreConstantIntExpr", IgnoreConstantIntExpr);
Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle());
}
@@ -84,6 +86,19 @@ void
ImplicitWideningOfMultiplicationResultCheck::handleImplicitCastExpr(
if (TgtWidth <= SrcWidth)
return;
+ // 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)) {
+ const auto TypeSize = Context->getTypeSize(ETy);
+ llvm::APSInt WidenedResult = ConstExprResult->extOrTrunc(TypeSize);
+ if (WidenedResult <= llvm::APSInt::getMaxValue(TypeSize, false) &&
+ WidenedResult >= llvm::APSInt::getMinValue(TypeSize, false))
+return;
+}
+ }
+
// Does the index expression look like it might be unintentionally computed
// in a narrower-than-wanted type?
const Expr *LHS = getLHSOfMulBinOp(E);
diff --git
a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
index 8b99930ae7a89..077a4b847cd9c 100644
---
a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
+++
b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
@@ -41,6 +41,7 @@ class ImplicitWideningOfMultiplicationResultCheck : public
ClangTidyCheck {
private:
const bool UseCXXStaticCastsInCppSources;
const bool UseCXXHeadersInCppSources;
+ const bool IgnoreConstantIntExpr;
utils::IncludeInserter IncludeInserter;
};
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
index c4ddd02602b73..6b857b9b82a21 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
+++
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
@@ -45,6 +45,12 @@ Options
should
[clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores const exprs that fit (PR #98352)
https://github.com/cwarner-8702 created
https://github.com/llvm/llvm-project/pull/98352
Add an option to the `bugprone-implicit-widening-of-multiplication-result`
clang-tidy checker to suppress warnings when the expression is made up of all
compile-time constants (literals, `constexpr` values or results, etc.) and the
result of the multiplication is guaranteed to fit in both the source and
destination types.
This currently only works for signed integer types:
* For unsigned integers, the well-defined rollover behavior on overflow
prevents the checker from detecting that the expression does not actually fit
in the source expr type, and would produce false negatives. I have a
somewhat-tacky stab at addressing this I'd like to submit as a follow-on PR
* For floating-point types, there's probably a little additional work to be
done to `Expr` to calculate the result at compile time
Even still, this seems like a helpful addition just for signed types, and
additional types can be added.
>From 49c39b058a75aa77abafd2422173e0b20accda4b Mon Sep 17 00:00:00 2001
From: Chris Warner
Date: Thu, 27 Jun 2024 14:14:43 -0700
Subject: [PATCH] [clang-tidy] bugprone-implicit-widening ignores const exprs
that fit
---
...citWideningOfMultiplicationResultCheck.cpp | 15
...licitWideningOfMultiplicationResultCheck.h | 1 +
...icit-widening-of-multiplication-result.rst | 6 ++
...ing-of-multiplication-result-constants.cpp | 81 +++
4 files changed, 103 insertions(+)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
index f99beac668ce7..46bf20e34ce04 100644
---
a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
+++
b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
@@ -42,6 +42,7 @@ ImplicitWideningOfMultiplicationResultCheck::
UseCXXStaticCastsInCppSources(
Options.get("UseCXXStaticCastsInCppSources", true)),
UseCXXHeadersInCppSources(Options.get("UseCXXHeadersInCppSources",
true)),
+ IgnoreConstantIntExpr(Options.get("IgnoreConstantIntExpr", false)),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()) {}
@@ -56,6 +57,7 @@ void
ImplicitWideningOfMultiplicationResultCheck::storeOptions(
Options.store(Opts, "UseCXXStaticCastsInCppSources",
UseCXXStaticCastsInCppSources);
Options.store(Opts, "UseCXXHeadersInCppSources", UseCXXHeadersInCppSources);
+ Options.store(Opts, "IgnoreConstantIntExpr", IgnoreConstantIntExpr);
Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle());
}
@@ -84,6 +86,19 @@ void
ImplicitWideningOfMultiplicationResultCheck::handleImplicitCastExpr(
if (TgtWidth <= SrcWidth)
return;
+ // 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)) {
+ const auto TypeSize = Context->getTypeSize(ETy);
+ llvm::APSInt WidenedResult = ConstExprResult->extOrTrunc(TypeSize);
+ if (WidenedResult <= llvm::APSInt::getMaxValue(TypeSize, false) &&
+ WidenedResult >= llvm::APSInt::getMinValue(TypeSize, false))
+return;
+}
+ }
+
// Does the index expression look like it might be unintentionally computed
// in a narrower-than-wanted type?
const Expr *LHS = getLHSOfMulBinOp(E);
diff --git
a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
index 8b99930ae7a89..077a4b847cd9c 100644
---
a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
+++
b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
@@ -41,6 +41,7 @@ class ImplicitWideningOfMultiplicationResultCheck : public
ClangTidyCheck {
private:
const bool UseCXXStaticCastsInCppSources;
const bool UseCXXHeadersInCppSources;
+ const bool IgnoreConstantIntExpr;
utils::IncludeInserter IncludeInserter;
};
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
index c4ddd02602b73..6b857b9b82a21 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
+++
