[clang-tools-extra] [clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result (PR #76315)

2024-01-13 Thread Piotr Zegar via cfe-commits 

https://github.com/PiotrZSL closed 
https://github.com/llvm/llvm-project/pull/76315
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result (PR #76315)

2024-01-12 Thread Félix-Antoine Constantin via cfe-commits 

felix642 wrote:

@PiotrZSL thank you for the review. 
I would greatly appreciate if you could merge this PR for me since I do not 
have the rights to do it.

https://github.com/llvm/llvm-project/pull/76315
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result (PR #76315)

2024-01-12 Thread Félix-Antoine Constantin via cfe-commits 


@@ -18,7 +18,7 @@ char *t0(char *base, int a, int b) {
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:17: note: perform multiplication in a wider 
type
   // CHECK-NOTES-C:(ptrdiff_t)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )

felix642 wrote:

I agree with you, like I've mentionned :

> Since both suggestions are overlapping clang-tidy refuses to apply them [...]

This is probably the reason why they are currently not using `CHECK-FIXES`. It 
would be a good idea to update the tests to use `CHECK-FIXES` but we would 
probably need to remove one of the FIX-IT for it to work.


https://github.com/llvm/llvm-project/pull/76315
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result (PR #76315)

2024-01-04 Thread Piotr Zegar via cfe-commits 

https://github.com/PiotrZSL approved this pull request.

LGTM.


https://github.com/llvm/llvm-project/pull/76315
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result (PR #76315)

2024-01-04 Thread Piotr Zegar via cfe-commits 


@@ -18,7 +18,7 @@ char *t0(char *base, int a, int b) {
   // CHECK-NOTES-CXX:  static_cast( )
   // CHECK-NOTES-ALL: :[[@LINE-5]]:17: note: perform multiplication in a wider 
type
   // CHECK-NOTES-C:(ptrdiff_t)
-  // CHECK-NOTES-CXX:  static_cast()
+  // CHECK-NOTES-CXX:  static_cast( )

PiotrZSL wrote:

NOTE: This is legacy issue and shoudn't be fixed under this PR, but all those 
checks with CHECK-NOTES looks invalid. We should use CHECK-MESSAGES and 
CHECK-FIXES, in that case we could verify that fixed line after applying fix 
looks correctly instead of checking clang-tidy output.

https://github.com/llvm/llvm-project/pull/76315
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result (PR #76315)

2024-01-04 Thread Piotr Zegar via cfe-commits 

https://github.com/PiotrZSL edited 
https://github.com/llvm/llvm-project/pull/76315
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result (PR #76315)

2023-12-23 Thread Félix-Antoine Constantin via cfe-commits 

felix642 wrote:

A patch that proposed a similar fix was previously submitted for this issue but 
never landed : https://reviews.llvm.org/D141058

https://github.com/llvm/llvm-project/pull/76315
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result (PR #76315)

2023-12-23 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang-tidy

Author: Félix-Antoine Constantin (felix642)


Changes

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_castuint64_t(1024 * )1024; }`
```
[source:5:12: warning: performing an implicit widening conversion to 
type 'uint64_t' (aka 'unsigned long') of a multiplication performed in type 
'int' [bugprone-implicit-widening-of-multiplication-result]](javascript:;)
5 | return 1024 * 1024;
  |^
[source:5:12: note: make conversion explicit to silence this 
warning](javascript:;)
1 | #include cstdint
2 | 
3 | uint64_t fn()
4 | {
5 | return 1024 * 1024;
  |^~~
  |static_castuint64_t( )
[source:5:12: note: perform multiplication in a wider 
type](javascript:;)
5 | return 1024 * 1024;
  |^~~~
  |static_castlong()
1 warning generated.
```

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: 
https://github.com/llvm/llvm-project/issues/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.

 

---
Full diff: https://github.com/llvm/llvm-project/pull/76315.diff


5 Files Affected:

- (modified) 
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
 (+19-8) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) 
- (modified) 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-array-subscript-expression.cpp
 (+2-2) 
- (modified) 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-int.cpp
 (+4-4) 
- (modified) 
clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-pointer-offset.cpp
 (+2-2) 


``diff
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 
 
 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(
+

[clang-tools-extra] [clang-tidy] Invalid Fix-It generated for implicit-widening-multiplication-result (PR #76315)

2023-12-23 Thread Félix-Antoine Constantin via cfe-commits 

https://github.com/felix642 created 
https://github.com/llvm/llvm-project/pull/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(1024 * )1024; }`
```
[:5:12: warning: performing an implicit widening conversion to type 
'uint64_t' (aka 'unsigned long') of a multiplication performed in type 'int' 
[bugprone-implicit-widening-of-multiplication-result]](javascript:;)
5 | return 1024 * 1024;
  |^
[:5:12: note: make conversion explicit to silence this 
warning](javascript:;)
1 | #include 
2 | 
3 | uint64_t fn()
4 | {
5 | return 1024 * 1024;
  |^~~
  |static_cast( )
[:5:12: note: perform multiplication in a wider type](javascript:;)
5 | return 1024 * 1024;
  |^~~~
  |static_cast()
1 warning generated.
```

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: 
https://github.com/llvm/llvm-project/issues/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.

 

From 7d9c4d423c00033e8c4df26b87176e3054976423 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A9lix-Antoine=20Constantin?=
 
Date: Sat, 23 Dec 2023 23:17:03 -0500
Subject: [PATCH] =?UTF-8?q?[clang-tidy]=C2=A0Invalid=20Fix-It=20generated?=
 =?UTF-8?q?=20for=20implicit-widening-multiplication-result?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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(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
---
 ...citWideningOfMultiplicationResultCheck.cpp | 27 +--
 clang-tools-extra/docs/ReleaseNotes.rst   |  4 +++
 ...tion-result-array-subscript-expression.cpp |  4 +--
 ...-widening-of-multiplication-result-int.cpp |  8 +++---
 ...f-multiplication-result-pointer-offset.cpp |  4 +--
 5 files changed, 31 insertions(+), 16 deletions(-)

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 
 
 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(),