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

Introduce clang-format + make the CI enforce clang-format clean C/C++ code #260

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

Conversation

hartwork
Copy link
Member

@hartwork hartwork commented Feb 6, 2023

@kaixiong the idea is this:

  • This is meant to be a draft, a demo, a place for discussion.
  • We initialize file .clang-format with the default formatting and then track changes to that file over time in Git.
  • Sorting of includes is disabled because of unpredictable effects, the rest is stock clang-format 16 and not a recommendation at detail level.
  • The pull request serves as both a demo of the approach and the configured options in detail, and adjusting and discussing .clang-format style choices until ideally the pain of everyone is low enough is part of what the pull request.
  • The mass-formatting commit has author clang-format <[email protected]> so it stands out in e.g. Git blame.
  • We would apply this pull request at a time when:
    • there are almost no or no other open pull requests and
    • there are almost no or no unported pull requests in either direction.

Looking forward to your feedback.

@kaixiong
Copy link
Member

@hartwork I would prefer to apply this first to Core only, extracting as much of the pre-existing style into the formatter configuration as possible. The configuration should focus on enforcing consistency instead of re-styling the existing code.

@hartwork
Copy link
Member Author

@hartwork I would prefer to apply this first to Core only

@kaixiong why only fix it in half the code?

, extracting as much of the pre-existing style into the formatter configuration as possible.

We can do that. If you can make a list of "bugs" in the effect, I can see if I can find the knows to turn for it.

The configuration should focus on enforcing consistency instead of re-styling the existing code.

Re-styling was never the key missing, it is a side-effect of the current inconsistency and in that sense a necessity, just the degree of it depends on the config.

@kaixiong
Copy link
Member

@hartwork Plugins that are imported code (actors mostly) come with their own formatting. Some are internally consistent, some are not. What's more, we mostly modify them just to fix bugs so I do not find the need for formatting fixes strong enough to mess up the blame history. Large patches like the one for DancingParticles are not common. I'm okay with applying Core's formatting to the others (input and morph).

@hartwork
Copy link
Member Author

I'm okay with applying Core's formatting to the others (input and morph).

@kaixiong is my understanding correct that actors starting from existing multi-file code would then not be discussed regarding whitespace except avoiding unnecessary changes in whitespace (since they are noise in diffs) and that we would auto-format all actor code that started in here just like core — would that be the deal?

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

Successfully merging this pull request may close these issues.

2 participants