[clang] [Sema] Fix fixit cast printing inside macros (PR #66853)

2023-09-20 Thread Shoaib Meenai via cfe-commits 

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

[clang] [Sema] Fix fixit cast printing inside macros (PR #66853)

2023-09-20 Thread Shoaib Meenai via cfe-commits 

https://github.com/smeenai updated 
https://github.com/llvm/llvm-project/pull/66853

>From 701b1d99515e40eec8dcbaba3a0b2dc436bf9652 Mon Sep 17 00:00:00 2001
From: Shoaib Meenai 
Date: Tue, 19 Sep 2023 19:46:56 -0700
Subject: [PATCH] [Sema] Fix fixit cast printing inside macros

`Lexer::getLocForEndOfToken` is documented as returning an invalid
source location when the end of the token is inside a macro expansion.
We don't want that for this particular application, so just calculate
the end location directly instead.

Before this, format fix-its would omit the closing parenthesis (thus
producing invalid code) for macros, e.g.:

```
$ cat format.cpp
extern "C" int printf(const char *, ...);
enum class Foo { Bar };
void f(Foo foo) { LOG("%d\n", foo); }

$ clang -fsyntax-only format.cpp
format.cpp:4:29: warning: format specifies type 'int' but the argument has type 
'Foo' [-Wformat]
4 | void f(Foo f) { LOG("%d\n", f); }
  |  ~~ ^
  | static_cast(
format.cpp:3:25: note: expanded from macro 'LOG'
3 | #define LOG(...) printf(__VA_ARGS__)
  | ^~~
1 warning generated.
```

We now emit a valid fix-it:

```
$ clang -fsyntax-only format.cpp
format.cpp:4:31: warning: format specifies type 'int' but the argument has type 
'Foo' [-Wformat]
4 | void f(Foo foo) { LOG("%d\n", foo); }
  |~~ ^~~
  |   static_cast( )
format.cpp:3:25: note: expanded from macro 'LOG'
3 | #define LOG(...) printf(__VA_ARGS__)
  | ^~~
1 warning generated.
```

Fixes https://github.com/llvm/llvm-project/issues/63462
---
 clang/lib/Sema/SemaChecking.cpp |  6 -
 clang/test/FixIt/format.cpp | 48 ++---
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index fad70223362eddd..b867d55c174e68b 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11470,7 +11470,11 @@ CheckPrintfHandler::checkFormatExpr(const 
analyze_printf::PrintfSpecifier ,
 Hints.push_back(
 FixItHint::CreateInsertion(E->getBeginLoc(), CastFix.str()));
 
-SourceLocation After = S.getLocForEndOfToken(E->getEndLoc());
+// We don't use getLocForEndOfToken because it returns invalid source
+// locations for macro expansions (by design).
+SourceLocation EndLoc = S.SourceMgr.getSpellingLoc(E->getEndLoc());
+SourceLocation After = EndLoc.getLocWithOffset(
+Lexer::MeasureTokenLength(EndLoc, S.SourceMgr, S.LangOpts));
 Hints.push_back(FixItHint::CreateInsertion(After, ")"));
   }
 
diff --git a/clang/test/FixIt/format.cpp b/clang/test/FixIt/format.cpp
index 9cc4c2600eb6670..5016ee587ed1c43 100644
--- a/clang/test/FixIt/format.cpp
+++ b/clang/test/FixIt/format.cpp
@@ -1,19 +1,61 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat %s
 // RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -Wformat %s 
2>&1 | FileCheck %s
 
 extern "C" int printf(const char *, ...);
+#define LOG(...) printf(__VA_ARGS__)
 
 namespace N {
   enum class E { One };
 }
 
-void a() {
+struct S {
+  N::E Type;
+};
+
+void a(N::E NEVal, S *SPtr, S ) {
   printf("%d", N::E::One); // expected-warning{{format specifies type 'int' 
but the argument has type 'N::E'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:25-[[@LINE-2]]:25}:")"
 
-  printf("%hd", N::E::One);
+  printf("%hd", N::E::One); // expected-warning{{format specifies type 'short' 
but the argument has type 'N::E'}}
   // CHECK: "static_cast("
 
-  printf("%hu", N::E::One);
+  printf("%hu", N::E::One); // expected-warning{{format specifies type 
'unsigned short' but the argument has type 'N::E'}}
   // CHECK: "static_cast("
+
+  LOG("%d", N::E::One); // expected-warning{{format specifies type 'int' but 
the argument has type 'N::E'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:13}:"static_cast("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:22-[[@LINE-2]]:22}:")"
+
+  printf("%d", NEVal); // expected-warning{{format specifies type 'int' but 
the argument has type 'N::E'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"static_cast("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:21-[[@LINE-2]]:21}:")"
+
+  LOG("%d", NEVal); // expected-warning{{format specifies type 'int' but the 
argument has type 'N::E'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:13}:"static_cast("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:")"
+
+  printf(
+  "%d",
+  SPtr->Type // expected-warning{{format specifies type 'int' but the 
argument has type 'N::E'}}
+  );
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:7}:"static_cast("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:17-[[@LINE-3]]:17}:")"
+
+  LOG( //

[clang] [Sema] Fix fixit cast printing inside macros (PR #66853)

2023-09-20 Thread Alex Brachet via cfe-commits 

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

Awesome! I couldn't figure this out when I looked :)

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

[clang] [Sema] Fix fixit cast printing inside macros (PR #66853)

2023-09-19 Thread Shoaib Meenai via cfe-commits 

smeenai wrote:

I updated the PR description with an example.

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

[clang] [Sema] Fix fixit cast printing inside macros (PR #66853)

2023-09-19 Thread Shoaib Meenai via cfe-commits 

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

[clang] [Sema] Fix fixit cast printing inside macros (PR #66853)

2023-09-19 Thread Timm Baeder via cfe-commits 

tbaederr wrote:

Since this is a diagnostic improvement, can you post the before and after 
output of an example?

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

[clang] [Sema] Fix fixit cast printing inside macros (PR #66853)

2023-09-19 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang


Changes

`Lexer::getLocForEndOfToken` is documented as returning an invalid
source location when the end of the token is inside a macro expansion.
We don't want that for this particular application, so just calculate
the end location directly instead.

Fixes https://github.com/llvm/llvm-project/issues/63462


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


2 Files Affected:

- (modified) clang/lib/Sema/SemaChecking.cpp (+4-1) 
- (modified) clang/test/FixIt/format.cpp (+5) 


``diff
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index fad70223362eddd..4fbdb315ddc87b4 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11470,7 +11470,10 @@ CheckPrintfHandler::checkFormatExpr(const 
analyze_printf::PrintfSpecifier ,
 Hints.push_back(
 FixItHint::CreateInsertion(E->getBeginLoc(), CastFix.str()));
 
-SourceLocation After = S.getLocForEndOfToken(E->getEndLoc());
+// We don't use getLocForEndOfToken because it returns invalid source
+// locations for macro expansions (by design).
+SourceLocation After = E->getEndLoc().getLocWithOffset(
+Lexer::MeasureTokenLength(E->getEndLoc(), S.SourceMgr, 
S.LangOpts));
 Hints.push_back(FixItHint::CreateInsertion(After, ")"));
   }
 
diff --git a/clang/test/FixIt/format.cpp b/clang/test/FixIt/format.cpp
index 9cc4c2600eb6670..46a0a0a11cc850a 100644
--- a/clang/test/FixIt/format.cpp
+++ b/clang/test/FixIt/format.cpp
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -Wformat %s 
2>&1 | FileCheck %s
 
 extern "C" int printf(const char *, ...);
+#define LOG(...) printf(__VA_ARGS__)
 
 namespace N {
   enum class E { One };
@@ -16,4 +17,8 @@ void a() {
 
   printf("%hu", N::E::One);
   // CHECK: "static_cast("
+
+  LOG("%d", N::E::One); // expected-warning{{format specifies type 'int' but 
the argument has type 'N::E'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:13}:"static_cast("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:22-[[@LINE-2]]:22}:")"
 }

``




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

[clang] [Sema] Fix fixit cast printing inside macros (PR #66853)

2023-09-19 Thread Shoaib Meenai via cfe-commits 

https://github.com/smeenai created 
https://github.com/llvm/llvm-project/pull/66853

`Lexer::getLocForEndOfToken` is documented as returning an invalid
source location when the end of the token is inside a macro expansion.
We don't want that for this particular application, so just calculate
the end location directly instead.

Fixes https://github.com/llvm/llvm-project/issues/63462


>From 66e12c0c192f6b490be5df694b2d7faedd1d1b74 Mon Sep 17 00:00:00 2001
From: Shoaib Meenai 
Date: Tue, 19 Sep 2023 19:46:56 -0700
Subject: [PATCH] [Sema] Fix fixit cast printing inside macros

`Lexer::getLocForEndOfToken` is documented as returning an invalid
source location when the end of the token is inside a macro expansion.
We don't want that for this particular application, so just calculate
the end location directly instead.

Fixes https://github.com/llvm/llvm-project/issues/63462
---
 clang/lib/Sema/SemaChecking.cpp | 5 -
 clang/test/FixIt/format.cpp | 5 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index fad70223362eddd..4fbdb315ddc87b4 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11470,7 +11470,10 @@ CheckPrintfHandler::checkFormatExpr(const 
analyze_printf::PrintfSpecifier ,
 Hints.push_back(
 FixItHint::CreateInsertion(E->getBeginLoc(), CastFix.str()));
 
-SourceLocation After = S.getLocForEndOfToken(E->getEndLoc());
+// We don't use getLocForEndOfToken because it returns invalid source
+// locations for macro expansions (by design).
+SourceLocation After = E->getEndLoc().getLocWithOffset(
+Lexer::MeasureTokenLength(E->getEndLoc(), S.SourceMgr, 
S.LangOpts));
 Hints.push_back(FixItHint::CreateInsertion(After, ")"));
   }
 
diff --git a/clang/test/FixIt/format.cpp b/clang/test/FixIt/format.cpp
index 9cc4c2600eb6670..46a0a0a11cc850a 100644
--- a/clang/test/FixIt/format.cpp
+++ b/clang/test/FixIt/format.cpp
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -Wformat %s 
2>&1 | FileCheck %s
 
 extern "C" int printf(const char *, ...);
+#define LOG(...) printf(__VA_ARGS__)
 
 namespace N {
   enum class E { One };
@@ -16,4 +17,8 @@ void a() {
 
   printf("%hu", N::E::One);
   // CHECK: "static_cast("
+
+  LOG("%d", N::E::One); // expected-warning{{format specifies type 'int' but 
the argument has type 'N::E'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:13}:"static_cast("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:22-[[@LINE-2]]:22}:")"
 }

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits