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

Add .clang-format file with Unit ruleset #1469

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

javorszky
Copy link
Contributor

@javorszky javorszky commented Oct 23, 2024

This adds a .clang-format file that can be used with the clang-format tool to either reformat code locally, or automatically flag up code styling issues during a github action.

This PR's only aim right now is to have a conversation about the rulesets and come to a standard (ie ruleset in the clang-format file) that the entire team agrees on, so we can get to a point where we're not having conversations about code styling in future PRs.

This PR does not aim to wire it up to github actions yet.

Diffs of the codebase:

@javorszky javorszky force-pushed the gabor/clang-format branch 2 times, most recently from db7097d to 3ad3d53 Compare October 23, 2024 19:40
@callahad
Copy link
Collaborator

@javorszky What's the current diff if you apply these rules to Unit?

@ac000
Copy link
Member

ac000 commented Oct 23, 2024

How was this file generated and where do all the comments come from?

@javorszky
Copy link
Contributor Author

How was this file generated and where do all the comments come from?

Originally with clang-format --dump-config --style=llvm > .clang-format, and then I went over every single rule there, looked up what they do on the documentation site, compared existing codebase to the values where I needed to make a decision.

Every comment is written by hand by me.

@ac000
Copy link
Member

ac000 commented Oct 23, 2024

Hmm

$ clang-format --dry-run src/nxt_php_sapi.c 
/home/andrew/src/unit/.clang-format:51:3: error: unknown key 'AlignFunctionDeclarations'
  AlignFunctionDeclarations: true
  ^~~~~~~~~~~~~~~~~~~~~~~~~
Error reading /home/andrew/src/unit/.clang-format: Invalid argument

@javorszky
Copy link
Contributor Author

need clang-format v20.0.0 sadly

@ac000
Copy link
Member

ac000 commented Oct 23, 2024

That's way too new!, it's not even released yet, probably in March next year...

@javorszky
Copy link
Contributor Author

I'll put up a new version with the v20 rules removed, and a diff for this, and all the other prebuilt styles as well

@javorszky
Copy link
Contributor Author

This has been updated so clang-format 19 will take it. I've also updated the main comment to point to a bunch of different draft PRs for the reformats. (if you don't see it, I'm still pushing / opening them)

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.

3 participants