Skip to content

PR for llvm/llvm-project#63462 #704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 29, 2023
Merged

PR for llvm/llvm-project#63462 #704

merged 1 commit into from
Sep 29, 2023

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Sep 21, 2023

`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 };
#define LOG(...) printf(__VA_ARGS__)
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<int>(
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<int>( )
format.cpp:3:25: note: expanded from macro 'LOG'
    3 | #define LOG(...) printf(__VA_ARGS__)
      |                         ^~~~~~~~~~~
1 warning generated.
```

Fixes llvm/llvm-project#63462

(cherry picked from commit 61c5ad8857a71510e4393680a1e42740da4ba6fa)
@smeenai
Copy link
Contributor

smeenai commented Sep 21, 2023

@tru would you consider this for the next 17 point release? -Wformat fires for enum classes in Clang 17, and it can be pretty high-firing. A fix-it was added by @abrachet in https://reviews.llvm.org/D153623 to make the migration less painful, but it didn't work correctly for macros (the fix-it would produce invalid code in that case). I tested this with a large internal codebase and it worked great.

@tru
Copy link
Contributor

tru commented Sep 28, 2023

WDYT @AaronBallman @erichkeane @cor3ntin

Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tru tru merged commit 45066b9 into release/17.x Sep 29, 2023
@tru tru deleted the llvm-issue63462 branch September 29, 2023 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expression end location not working in macros
4 participants