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

Everywhere: Reformat using clang-format 19 #25075

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DanShaders
Copy link
Contributor

@DanShaders DanShaders commented Oct 4, 2024

We run CI to test changes, so what's the point of doing that locally?

This also fixes an accidental install-remove-install dance on Lagom
jobs.
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member labels Oct 4, 2024
@DanShaders DanShaders force-pushed the clang-format-19 branch 2 times, most recently from 5279e78 to c51410b Compare October 4, 2024 22:53
The next major version of LLVM brings new formatting for
pointers-to-members, inline assembly blocks and some empty blocks.

The commit was generated using
```
find AK Kernel Ladybird Meta Tests Userland \
  \( -iname '*.cpp' -or -iname '*.h' \) \
  -exec Toolchain/Local/clang/bin/clang-format -i '{}' ';'
```
with a few manual corrections.
@timschumi
Copy link
Member

I feel like we might want to hold off on this. Apart from the one fix in the last commit, the changes mostly look like changes that we may want to revert again once clang-format gains the respective option.

@DanShaders
Copy link
Contributor Author

Hmm, @ADKaster and @spholz liked new pointer-to-member formatting and assembly formatting, respectively.
Personally, I don't really care about pointers-to-member or empty blocks (but I like that that strange {}; construct no longer fools clang-format) but inline assembly formatting always looked very weird to me (and I actually glad it changed).

@Hendiadyoin1
Copy link
Contributor

SideNote: still waiting for clang format to allow us to enable "align consecutive assignments" for enums only

@timschumi
Copy link
Member

Hmm, @ADKaster and @spholz liked new pointer-to-member formatting and assembly formatting, respectively. Personally, I don't really care about pointers-to-member or empty blocks (but I like that that strange {}; construct no longer fools clang-format) but inline assembly formatting always looked very weird to me (and I actually glad it changed).

pointer-to-member is something I hadn't seen until now, and I generally don't do much with inline assembly, so I don't really have a strong opinion on those two (I just assumed that assembly looked a bit nicer when aligned).

However, for the {} constructs, it seems like we are strongly preferring {} over { } based on pure usage counts:

$ ag 'return {}' | wc -l
6629
$ ag 'return { }' | wc -l
2
$ ag '{};' | wc -l
10607
$ ag '{ };' | wc -l
70

@DanShaders
Copy link
Contributor Author

DanShaders commented Oct 17, 2024

The empty block changes are due to a bug fix that made SpaceInEmptyBlock: true from WebKit preset actually work. New rule in clang-format and empty blocks revert incoming.

See discussion on Discord

@nico
Copy link
Contributor

nico commented Oct 18, 2024

Re {}: In function decls with empty bodies, clang-format currently produces { } with a space – unless there's an incorrect and needless ; after the brace. Then it used to produce {};. Now it knows that the ; is silly and removes it, and the formats as if it wasn't there. That's a (big!) progression.

So I think if we do want to reformat every 6 months, then this looks like a fairly clean reformat as-is.

@nico
Copy link
Contributor

nico commented Oct 21, 2024

Maybe it'd be nice to omit LibWeb here. That's mostly cherry-picks at the moment, and this here will make those even harder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants