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 identifies empty function as bracedList #58251

Closed
mydeveloperday opened this issue Oct 9, 2022 · 7 comments · Fixed by #72733
Closed

[clang-format] Incorrectly identifies empty function as bracedList #58251

mydeveloperday opened this issue Oct 9, 2022 · 7 comments · Fixed by #72733
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior clang-format

Comments

@mydeveloperday
Copy link
Contributor

Because clang-format cannot determine {} as an empty function is leaves out a space between the ) and the { as it doesn't identify the { as a TT_FunctionLBrace

Add an issue for tracking a potential change to fix that.

void function(){};
void function() { int a; };
@mydeveloperday mydeveloperday added bug Indicates an unexpected problem or unintended behavior clang-format labels Oct 9, 2022
@llvmbot
Copy link
Member

llvmbot commented Oct 9, 2022

@llvm/issue-subscribers-bug

@llvmbot
Copy link
Member

llvmbot commented Oct 9, 2022

@llvm/issue-subscribers-clang-format

@mydeveloperday
Copy link
Contributor Author

mydeveloperday commented Oct 9, 2022

While working on a fix for #58217 (namely https://reviews.llvm.org/D135466) we identified that foo(){} was not being identified the same as foo() { return; }

@rymiel
Copy link
Member

rymiel commented Oct 9, 2022

Also see #57305 (comment). Not exactly a duplicate but the same underlying bug, i'm assuming

@mydeveloperday
Copy link
Contributor Author

The more I look at this the more I think this is harder than we first think, simply assuming ) { to be a function would change, would then identify the { as a FunctionLBrace, which in turn RemoveSemicolons would remove the trailing ; which is needed i.e.

this

auto i = decltype(x) {};

to this

auto i = decltype(x) {}

which would be a compile error

So we'd really need yet more semantic (or derived semantic) information about what makes {}; a function body.

The unwrappered parser when we are determining the block type is before the annotator which identified the ')' as a TypeDeclarationParen, but this would be useful information to have.

I wonder if there is something we can do after the function name has been identified as FunctionDeclarationName

At present we get this..

Expected & Input

void a() {}
void a() {};
void a() { return a;}
void a() { return a;};

Clang-Format current output

void a() {}
void a(){};
void a() { return a; }
void a() { return a; };

@rymiel
Copy link
Member

rymiel commented Oct 9, 2022

The more I look at this the more I think this is harder than we first think

I definitely couldn't fix it. I concluded that determining if a function declaration is being dealt with (i.e. Line.MightBeFunctionDecl) happens way later and I couldn't find a non-invasive heuristic that didn't involve lots of duplicated work

@mydeveloperday
Copy link
Contributor Author

feel free to have a go.

@owenca owenca self-assigned this Nov 17, 2023
owenca added a commit to owenca/llvm-project that referenced this issue Nov 18, 2023
Also fixed some existing test cases.

Fixed llvm#57305.
Fixed llvm#58251.
owenca added a commit that referenced this issue Nov 19, 2023
Also fixed some existing test cases.

Fixed #57305.
Fixed #58251.
sr-tream pushed a commit to sr-tream/llvm-project that referenced this issue Nov 20, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this issue Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior clang-format
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants