Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ci: run clang-tidy as a part of CI #4666
ci: run clang-tidy as a part of CI #4666
Changes from 6 commits
00fb3b5
ce3d2c6
81565c3
48e6eb3
9a94f10
fac609e
3441099
67bea26
f55bfd3
1406e3b
b1936dc
7817f46
8ad5da9
125ba28
8078e74
6904c85
7cd3c85
cf09277
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@envoyproxy/maintainers I am open to any suggestion to this file. With this
.clang-tidy
it still issue too many warnings. A result from a couple days ago:https://gist.github.com/lizan/115f05ace9ac7dbae4c042f6fbdb3596
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Don't we need to fail if clang-tidy fails? So don't we need to be warning clean? Or we only fail on errors? In general it seems like we should be warning clean also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I don't think we should mark clang-tidy required, at least in a meanwhile. We can make some of checks error with
-warnings-as-errors
later to fail on them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then what's the point of using a CI slot for this? No one will look at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like mac CI that is not required (on GitHub) but we still check? At this point, there are too many warnings to fix at once so I am hesitating to make them error.
cc @jsedgwick @rlazarus @PiotrSikora on #921, any suggestion to enable some checks as
WarningsAsErrors
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does Google deal with this internally? Since Google fixes bugs raised by clang-tidy presumably some internal CI is catching them? Can we just start with Google's config? cc @htuch @alyssawilk @PiotrSikora
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally clang-tidy is largely advisory I believe, but most reviewers will take its suggestions as actionable. For Envoy, only some importers (we have a weekly rotation) actually pay attention as it's largely mechanical.
I think we have a noise problem with a purely advisory clang-tidy, if the report is as noisy as in https://gist.github.com/lizan/115f05ace9ac7dbae4c042f6fbdb3596, reviewers will never both to look at it. Inside Google, our review system highlights issues only in the delta diff.
Maybe we can turn off all warnings, make clang-tidy compulsory and only turns on the ones that actually are low on the false positive side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes running clang-tidy on diff (especially for PRs) is definitely the plan, considering how clang-tidy is heavy (it takes an hour to run on whole source code). There is a tool to run clang-tidy against diff.
If we do have a better .clang-tidy to start with, I'm happy to start with that rather than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, IMO we need to make this actionable or we shouldn't bother running it in CI.