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 and run it on the code-base #384

Merged
merged 6 commits into from
Mar 7, 2024
Merged

Add clang-format and run it on the code-base #384

merged 6 commits into from
Mar 7, 2024

Conversation

erlingrj
Copy link
Collaborator

@erlingrj erlingrj commented Mar 5, 2024

This PR replaces #310. It copies clang-format setup from reactor-cpp. I have added a simple Makefile target so to format reactor-c run:

make format

In CI the following check is run which returns an error if it finds anything it would like to change

make format-check

For any outstanding feature branch I suggest the following strategy:

  1. Cherry-pick the commit adding clang-format: git cherry-pick 91bc46cd17cbd12eaaa83f4684b3568341f79d6d
  2. Run the formatter on the feature branch. This must be done several times until clang-format "stabilizes" on a valid format:
make format
make format

Then hopefully there will be few conflicts

@erlingrj erlingrj requested a review from edwardalee March 5, 2024 10:01
@erlingrj
Copy link
Collaborator Author

erlingrj commented Mar 5, 2024

Tagging the most frequent contributors to reactor-c here for your information: @petervdonovan @ChadliaJerad @lhstrh @lsk567 Anyone else that is working on major features?

Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's do this and get the pain over with.

Copy link
Contributor

@petervdonovan petervdonovan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I'm on board with this. This will probably make CONTRIBUTING.md, which was never really used anyway, out of date. We should probably just delete CONTRIBUTING.md.

EDIT: This reminds me, if there is a way to do this that will make it easier to reach partial compliance with a standard like MISRA (that is, compliance with the parts that make sense for us), then that might be worth considering. But I have no expertise on that and I'm sure you've already considered that, so I'm not going to try to make a lot of noise about it.

@erlingrj
Copy link
Collaborator Author

erlingrj commented Mar 6, 2024

@petervdonovan regarding MISRA. I think that goes beyond formatting and is more a topic of linting? There is a static analysis/linting tool called clang-tidy that we could integrate also. I do not know a lot on this topic though.

@erlingrj erlingrj added this pull request to the merge queue Mar 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 7, 2024
@lhstrh lhstrh enabled auto-merge March 7, 2024 06:16
@erlingrj
Copy link
Collaborator Author

erlingrj commented Mar 7, 2024

Thank you @lhstrh!

@lhstrh
Copy link
Member

lhstrh commented Mar 7, 2024

Thank you @lhstrh!

Sorry it took so long... No need for this to hold you up either. The tests are clearly passing, so it's also fine to bypass the merge queue and merge directly this time.

@lhstrh lhstrh added this pull request to the merge queue Mar 7, 2024
Merged via the queue into main with commit 28a97ae Mar 7, 2024
30 checks passed
@erlingrj erlingrj deleted the clang-format branch May 6, 2024 09:12
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.

4 participants