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

Improve clang-tidy check #798

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

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Oct 20, 2022

Partially fixes #796:

  • Don't -export-fixes by default
  • Use run-clang-tidy instead of clang-tidy to (hopefully) better handle parallel changes to the same files
    Unfortunately, this changes cmdline API, i.e. options have different names!

TODO: Handle fixes from multiple packages:

There is an interesting github action, processing clang-tidy's fixes into PR comments:
https://github.com/marketplace/actions/pull-request-comments-from-clang-tidy-reports

I triggered a test build for MoveIt: moveit/moveit#3250

@EzraBrooks, do you have resources to contribute?

- Use run-clang-tidy to run parallel jobs
- Drop -export-fixes argument to allow users to specify a location
@rhaschke rhaschke marked this pull request as draft October 20, 2022 13:44
@EzraBrooks
Copy link

I'd love to help, but we're currently swamped here with customer deliverables and conference demos. It's that time of the (fiscal) year! Thanks for your work on this.

@rhaschke rhaschke force-pushed the clang-tidy branch 3 times, most recently from 586f85d to 41ee805 Compare November 3, 2022 13:52
@rhaschke rhaschke marked this pull request as ready for review November 3, 2022 16:05
@rhaschke
Copy link
Contributor Author

rhaschke commented Nov 3, 2022

I finally found some time to finish this. Here is an example output. Note, the last commit is needed for proper links on github.

@EzraBrooks
Copy link

This is awesome! I'm going to make a PR on my project to use this branch of ICI and test it out.

@EzraBrooks
Copy link

Works like a charm!

@mathias-luedtke mathias-luedtke self-requested a review November 3, 2022 17:28
@tylerjw
Copy link
Contributor

tylerjw commented Nov 30, 2022

@mathias-luedtke we are using this, and it works well for us. Would you mind reviewing this?

@rhaschke
Copy link
Contributor Author

Closing and reopening to re-trigger CI. The failure was unrelated to this PR.
@mathias-luedtke, is there any other reason holding this back?
This PR is not just cosmetic, but actually fixes a bug with clang-tidy generated fixes: The old approach, running multiple tidy instances that fix the code in parallel, caused conflicts.

@rhaschke rhaschke closed this Feb 16, 2023
@rhaschke rhaschke reopened this Feb 16, 2023
@EzraBrooks
Copy link

This has remained stable in my team's CI and has been excellent in conjunction with this action that highlights our clang-tidy violations in our PR diffs.

Copy link
Member

@130s 130s left a comment

Choose a reason for hiding this comment

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

CI is green, downstream projects are +1-ing (and longing for this change to be merged). So I'm +1-ing too. Haven't taken a deeper look though. Deferring to @mathias-luedtke

@130s
Copy link
Member

130s commented Jun 3, 2023

@mathias-luedtke Would you mind reviewing this?

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.

Allow output of clang-tidy fixes file
4 participants