Skip to content
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

[clang-format] Incorrectly formatted macro #78965

Closed
prj- opened this issue Jan 22, 2024 · 10 comments · Fixed by #79549
Closed

[clang-format] Incorrectly formatted macro #78965

prj- opened this issue Jan 22, 2024 · 10 comments · Fixed by #79549

Comments

@prj-
Copy link

prj- commented Jan 22, 2024

$ cat foo.c 
#define foo(i, j)     (i - bar(j))
#define foo(i, j)     ((i) - bar(j))
$ clang-format --version
clang-format version 17.0.6
$ clang-format foo.c    
#define foo(i, j) (i - bar(j))
#define foo(i, j) ((i)-bar(j))
$ clang-format-18 --version
Debian clang-format version 18.0.0 (++20240119100743+cd05ade13a66-1~exp1~20240119220916.1830)
$ clang-format-18 foo.c 
#define foo(i, j) (i - bar(j))
#define foo(i, j) ((i)-bar(j))

Shouldn't the proper output be the following?

#define foo(i, j) (i - bar(j))
#define foo(i, j) ((i) - bar(j))
@rymiel
Copy link
Member

rymiel commented Jan 22, 2024

It is being interpreted as a unary minus on the call (-bar(j)), being casted to i, due to ambiguities between types and variables in macros

AnnotatedTokens(L=1, P=0, T=5, C=0):
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=l_paren L=1 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=59 Name=l_paren L=2 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=identifier L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x555bc6dc45a0 Text='i'
 M=0 C=0 T=CastRParen S=0 F=0 B=0 BK=0 P=63 Name=r_paren L=4 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=UnaryOperator S=0 F=0 B=0 BK=0 P=140 Name=minus L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='-'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=100 Name=identifier L=8 PPK=2 FakeLParens= FakeRParens=0 II=0x555bc6dc4600 Text='bar'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=l_paren L=9 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=identifier L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x555bc6dc45d0 Text='j'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=63 Name=r_paren L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'

@prj-
Copy link
Author

prj- commented Jan 22, 2024

OK, I can bypass the issue with #define foo(i, j) (-bar(j)+(i)). If you don't think this is an issue and there is no way to resolve the ambiguity on your end, feel free to close the issue.

@prj-
Copy link
Author

prj- commented Jan 23, 2024

Could you please let me know, @rymiel?

@rymiel
Copy link
Member

rymiel commented Jan 23, 2024

I think improvements can always be made, I'd keep it open for now, but I wouldn't expect it to be resolved any time soon

@prj-
Copy link
Author

prj- commented Jan 27, 2024

Wow, it got fixed earlier than I expected, thanks to you both!

llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 4, 2024
@owenca owenca reopened this Feb 4, 2024
@owenca owenca added this to the LLVM 18.X Release milestone Feb 4, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Feb 4, 2024
@owenca
Copy link
Contributor

owenca commented Feb 4, 2024

/cherry-pick f826f55

llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 4, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 4, 2024

/pull-request #80593

@tstellar
Copy link
Collaborator

tstellar commented Feb 5, 2024

PR has been created, we will track the status there.

@tstellar tstellar closed this as completed Feb 5, 2024
@tstellar tstellar moved this from Needs Triage to Done in LLVM Release Status Feb 5, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 7, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
@andreyv
Copy link
Member

andreyv commented Apr 10, 2024

It looks like f826f55 caused a regression in macro formatting:

#define MAX ((uint32_t)-1)

now gets formatted as

#define MAX ((uint32_t) - 1)

while ClangFormat 17 leaves it as is. This commit was backported to stable releases, so 18.1.3 also exhibits the problem.

Both versions still correctly recognize

uint32_t max = (uint32_t)-1;

and don't change it.

@owenca
Copy link
Contributor

owenca commented Apr 11, 2024

This was fixed in 0baef3b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

6 participants