[clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result #76315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ation-result
The check currently emits warnings for the following code:
uint64_t fn() { return 1024 * 1024; }
But the code generated after applying the notes will look like this:
uint64_t fn() { return static_cast<uint64_t>(1024 * )1024; }
This is because when generating the notes the check will use the beginLoc
and EndLoc of the subexpr of the implicit cast.
But in some cases the AST Node might not have a beginLoc and EndLoc. This
seems to be true when the Node is composed of only 1 token (for example
an integer literal). Calling the getEndLoc() on this type of node will
simply return the known location which is, in this case, the beginLoc.
Fixes llvm#63070 llvm#56728
|
@llvm/pr-subscribers-clang-tidy Author: Félix-Antoine Constantin (felix642) ChangesThe check currently emits warnings for the following code: But the code generated after applying the notes will look like this: This is because when generating the notes the check will use the beginLoc() and EndLoc() of the subexpr of the implicit cast. Fixes #63070 #56728 I was not able to add tests for this commit and this is because the check currently emits two notes for every detection. Since both suggestions are overlapping clang-tidy refuses to apply them and I was not able to add new tests. The first suggestion seems useless since we should not suggest a fix-it to silence an issue (the message seems sufficient to me). Full diff: https://github.com/llvm/llvm-project/pull/76315.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
index 95a7c521eabb0e..6f22f02f301835 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
@@ -9,6 +9,7 @@
#include "ImplicitWideningOfMultiplicationResultCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
#include <optional>
using namespace clang::ast_matchers;
@@ -99,15 +100,16 @@ void ImplicitWideningOfMultiplicationResultCheck::handleImplicitCastExpr(
"make conversion explicit to silence this warning",
DiagnosticIDs::Note)
<< E->getSourceRange();
-
+ const SourceLocation EndLoc = Lexer::getLocForEndOfToken(
+ E->getEndLoc(), 0, *Result->SourceManager, getLangOpts());
if (ShouldUseCXXStaticCast)
Diag << FixItHint::CreateInsertion(
E->getBeginLoc(), "static_cast<" + Ty.getAsString() + ">(")
- << FixItHint::CreateInsertion(E->getEndLoc(), ")");
+ << FixItHint::CreateInsertion(EndLoc, ")");
else
Diag << FixItHint::CreateInsertion(E->getBeginLoc(),
"(" + Ty.getAsString() + ")(")
- << FixItHint::CreateInsertion(E->getEndLoc(), ")");
+ << FixItHint::CreateInsertion(EndLoc, ")");
Diag << includeStddefHeader(E->getBeginLoc());
}
@@ -137,7 +139,11 @@ void ImplicitWideningOfMultiplicationResultCheck::handleImplicitCastExpr(
Diag << FixItHint::CreateInsertion(LHS->getBeginLoc(),
"static_cast<" +
WideExprTy.getAsString() + ">(")
- << FixItHint::CreateInsertion(LHS->getEndLoc(), ")");
+ << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(LHS->getEndLoc(), 0,
+ *Result->SourceManager,
+ getLangOpts()),
+ ")");
else
Diag << FixItHint::CreateInsertion(LHS->getBeginLoc(),
"(" + WideExprTy.getAsString() + ")");
@@ -206,16 +212,17 @@ void ImplicitWideningOfMultiplicationResultCheck::handlePointerOffsetting(
"make conversion explicit to silence this warning",
DiagnosticIDs::Note)
<< IndexExpr->getSourceRange();
-
+ const SourceLocation EndLoc = Lexer::getLocForEndOfToken(
+ IndexExpr->getEndLoc(), 0, *Result->SourceManager, getLangOpts());
if (ShouldUseCXXStaticCast)
Diag << FixItHint::CreateInsertion(
IndexExpr->getBeginLoc(),
(Twine("static_cast<") + TyAsString + ">(").str())
- << FixItHint::CreateInsertion(IndexExpr->getEndLoc(), ")");
+ << FixItHint::CreateInsertion(EndLoc, ")");
else
Diag << FixItHint::CreateInsertion(IndexExpr->getBeginLoc(),
(Twine("(") + TyAsString + ")(").str())
- << FixItHint::CreateInsertion(IndexExpr->getEndLoc(), ")");
+ << FixItHint::CreateInsertion(EndLoc, ")");
Diag << includeStddefHeader(IndexExpr->getBeginLoc());
}
@@ -229,7 +236,11 @@ void ImplicitWideningOfMultiplicationResultCheck::handlePointerOffsetting(
Diag << FixItHint::CreateInsertion(
LHS->getBeginLoc(),
(Twine("static_cast<") + TyAsString + ">(").str())
- << FixItHint::CreateInsertion(LHS->getEndLoc(), ")");
+ << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(IndexExpr->getEndLoc(), 0,
+ *Result->SourceManager,
+ getLangOpts()),
+ ")");
else
Diag << FixItHint::CreateInsertion(LHS->getBeginLoc(),
(Twine("(") + TyAsString + ")").str());
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6d5f49dc062545..55fba7d86468d0 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -222,6 +222,10 @@ Changes in existing checks
casting during type conversions at variable initialization, now with improved
compatibility for C++17 and later versions.
+- Improved :doc:`bugprone-implicit-widening-of-multiplication-result
+ <clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result>` check
+ to correctly emit fixes.
+
- Improved :doc:`bugprone-lambda-function-name
<clang-tidy/checks/bugprone/lambda-function-name>` check by adding option
`IgnoreMacros` to ignore warnings in macros.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
index 2881341c878570..5b432cb2662968 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
@@ -18,7 +18,7 @@ char *t0(char *base, int a, int b) {
// CHECK-NOTES-CXX: static_cast<ptrdiff_t>( )
// CHECK-NOTES-ALL: :[[@LINE-5]]:16: note: perform multiplication in a wider type
// CHECK-NOTES-C: (ptrdiff_t)
- // CHECK-NOTES-CXX: static_cast<ptrdiff_t>()
+ // CHECK-NOTES-CXX: static_cast<ptrdiff_t>( )
}
void *t1(char *base, int a, int b) {
return &((a * b)[base]);
@@ -35,7 +35,7 @@ char *t2(char *base, unsigned int a, int b) {
// CHECK-NOTES-CXX: static_cast<size_t>( )
// CHECK-NOTES-ALL: :[[@LINE-5]]:16: note: perform multiplication in a wider type
// CHECK-NOTES-C: (size_t)
- // CHECK-NOTES-CXX: static_cast<size_t>()
+ // CHECK-NOTES-CXX: static_cast<size_t>( )
}
char *t3(char *base, int a, unsigned int b) {
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
index cf13a7ea9fbd8b..8619a4ee86550a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
@@ -18,7 +18,7 @@ long t0(int a, int b) {
// CHECK-NOTES-CXX: static_cast<long>( )
// CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
// CHECK-NOTES-C: (long)
- // CHECK-NOTES-CXX: static_cast<long>()
+ // CHECK-NOTES-CXX: static_cast<long>( )
}
unsigned long t1(int a, int b) {
return a * b;
@@ -28,7 +28,7 @@ unsigned long t1(int a, int b) {
// CHECK-NOTES-CXX: static_cast<unsigned long>( )
// CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
// CHECK-NOTES-C: (long)
- // CHECK-NOTES-CXX: static_cast<long>()
+ // CHECK-NOTES-CXX: static_cast<long>( )
}
long t2(unsigned int a, int b) {
@@ -39,7 +39,7 @@ long t2(unsigned int a, int b) {
// CHECK-NOTES-CXX: static_cast<long>( )
// CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
// CHECK-NOTES-C: (unsigned long)
- // CHECK-NOTES-CXX: static_cast<unsigned long>()
+ // CHECK-NOTES-CXX: static_cast<unsigned long>( )
}
unsigned long t3(unsigned int a, int b) {
return a * b;
@@ -49,7 +49,7 @@ unsigned long t3(unsigned int a, int b) {
// CHECK-NOTES-CXX: static_cast<unsigned long>( )
// CHECK-NOTES-ALL: :[[@LINE-5]]:10: note: perform multiplication in a wider type
// CHECK-NOTES-C: (unsigned long)
- // CHECK-NOTES-CXX: static_cast<unsigned long>()
+ // CHECK-NOTES-CXX: static_cast<unsigned long>( )
}
long t4(int a, unsigned int b) {
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
index c812885507e8f3..5ba6a5639849df 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
@@ -18,7 +18,7 @@ char *t0(char *base, int a, int b) {
// CHECK-NOTES-CXX: static_cast<ptrdiff_t>( )
// CHECK-NOTES-ALL: :[[@LINE-5]]:17: note: perform multiplication in a wider type
// CHECK-NOTES-C: (ptrdiff_t)
- // CHECK-NOTES-CXX: static_cast<ptrdiff_t>()
+ // CHECK-NOTES-CXX: static_cast<ptrdiff_t>( )
}
char *t1(char *base, int a, int b) {
return a * b + base;
@@ -35,7 +35,7 @@ char *t2(char *base, unsigned int a, int b) {
// CHECK-NOTES-CXX: static_cast<size_t>( )
// CHECK-NOTES-ALL: :[[@LINE-5]]:17: note: perform multiplication in a wider type
// CHECK-NOTES-C: (size_t)
- // CHECK-NOTES-CXX: static_cast<size_t>()
+ // CHECK-NOTES-CXX: static_cast<size_t>( )
}
char *t3(char *base, int a, unsigned int b) {
|
|
A patch that proposed a similar fix was previously submitted for this issue but never landed : https://reviews.llvm.org/D141058 |
PiotrZSL
approved these changes
Jan 4, 2024
...t/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
Show resolved
Hide resolved
|
@PiotrZSL thank you for the review. |
justinfargnoli
pushed a commit
to justinfargnoli/llvm-project
that referenced
this pull request
Jan 28, 2024
…ation-result (llvm#76315) The check currently emits warnings for the following code: `uint64_t fn() { return 1024 * 1024; }` But the code generated after applying the notes will look like this: `uint64_t fn() { return static_cast<uint64_t>(1024 * )1024; }` This is because when generating the notes the check will use the beginLoc() and EndLoc() of the subexpr of the implicit cast. But in some cases the AST Node might not have a beginLoc and EndLoc. This seems to be true when the Node is composed of only 1 token (for example an integer literal). Calling the getEndLoc() on this type of node will simply return the known location which is, in this case, the beginLoc. Fixes llvm#63070 llvm#56728
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The check currently emits warnings for the following code:
uint64_t fn() { return 1024 * 1024; }But the code generated after applying the notes will look like this:
uint64_t fn() { return static_cast<uint64_t>(1024 * )1024; }This is because when generating the notes the check will use the beginLoc() and EndLoc() of the subexpr of the implicit cast.
But in some cases the AST Node might not have a beginLoc and EndLoc. This seems to be true when the Node is composed of only 1 token (for example an integer literal). Calling the getEndLoc() on this type of node will simply return the known location which is, in this case, the beginLoc.
Fixes #63070 #56728
I was not able to add tests for this commit and this is because the check currently emits two notes for every detection.
The first suggestion is to silence the warning by casting the whole operation.
The second suggestion is there to fix the issue (And I think the suggested code is invalid, but I've opened another issue for that: #76311).
Since both suggestions are overlapping clang-tidy refuses to apply them and I was not able to add new tests.
The first suggestion seems useless since we should not suggest a fix-it to silence an issue (the message seems sufficient to me).
But this requires more discussion and the change is not trivial so I've left it how it is for now.