-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Only run clang-tidy warning checks reported as errors #36885
Conversation
That didn't work as expected,
Edit: never mind there was a stray quote and so a parse error. |
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.
Landing this as-is will break https://github.com/flutter/engine/pull/36910/files since the checks will be specified as errors for mac, but not be turned on as checks. Clang-tidy isn't smart about that. We need to make our clang-tidy app account for that or keep the checks on in the .clang-tidy
file.
I think we should keep it on as a check since it helps out in limited cases. If we still want to remove it I can update the clang-tidy tool first. I don't want to put more work on your plate. We just have to make a decision @zanderso @jmagman.
From Triage: We were wondering if this can be resurrected now. Let's close it if not. In the meantime, I am marking this as a WIP. |
All of the checks that were removed here that have clang-tidy fix implementations can probably be turned on as errors now since I've ran clang-tidy fix to land the performance lint checks and they got cleaned up. For example the removal of c style casts should be complete now. |
@gaaclarke I still see three on Linux host
( Filed flutter/flutter#115518 |
|
Yea, sorry I meant at the time of landing that PR they should all have been fixed, but there is nothing stoping future regressions since they are not turned on as errors. We should turn them on as errors. |
That's the point of this PR, to make all checks we have on show up as errors, otherwise they won't show up in CI and regressions are inevitable. Those four checks still have issues though, so we will need to turn them off until the issues are fixed. |
8139218
to
bb38813
Compare
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.
lgtm!
Gold has detected about 1 new digest(s) on patchset 4. |
…115614) * 1f4ddc3ee Only run clang-tidy warning checks reported as errors (flutter/engine#36885) * e812122e4 Roll Fuchsia Linux SDK from JzUXGxIUUNgr8qJUx... to lnmSnyJi-2H07tBnV... (flutter/engine#37729)
…lutter#115614) * 1f4ddc3ee Only run clang-tidy warning checks reported as errors (flutter/engine#36885) * e812122e4 Roll Fuchsia Linux SDK from JzUXGxIUUNgr8qJUx... to lnmSnyJi-2H07tBnV... (flutter/engine#37729)
…lutter#115614) * 1f4ddc3ee Only run clang-tidy warning checks reported as errors (flutter/engine#36885) * e812122e4 Roll Fuchsia Linux SDK from JzUXGxIUUNgr8qJUx... to lnmSnyJi-2H07tBnV... (flutter/engine#37729)
lint.sh
was runningclang-tidy
but only failing on the warnings explicitly treated as errors. That means it was running more checks than it was reporting, but not giving any additional information.Treat all clang-tidy warnings as errors. This will also prevent checks being added that aren't really being reported.
Remove
performance-move-const-arg
andperformance-unnecessary-value-param
checks because they produce errors that were never surfaced and would need to be fixed before these checks are re-added.It looks like this improved
Mac Host clang-tidy
time, but the rest are about the same.Pre-launch Checklist
writing and running engine tests.
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.