[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

Only run clang-tidy warning checks reported as errors #36885

Merged
merged 1 commit into from
Nov 18, 2022
Merged

Only run clang-tidy warning checks reported as errors #36885

merged 1 commit into from
Nov 18, 2022

Conversation

jmagman
Copy link
Member
@jmagman jmagman commented Oct 20, 2022 •
edited

lint.sh was running clang-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 and performance-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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@jmagman jmagman self-assigned this Oct 20, 2022
@jmagman
Copy link
Member Author
jmagman commented Oct 20, 2022 •
edited

That didn't work as expected, --warnings-as-errors is ignoring the disabled checks even though it shouldn't:

FlutterBinaryMessengerRelay.mm:42:5: error: nil returned from a method that is expected to return a non-null value [clang-analyzer-nullability.NullReturnedFromNonnull,-warnings-as-errors]

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8799827550243419073/+/u/test:_lint_ios_debug_sim/stdout

Checks: "bugprone-use-after-move,\
...
-clang-analyzer-nullability.NullReturnedFromNonnull,\

  --warnings-as-errors=<string>  -
                                   Upgrades warnings to errors. Same format as
                                   '-checks'.
                                   This option's value is appended to the value of
                                   the 'WarningsAsErrors' option in .clang-tidy
                                   file, if any.

Edit: never mind there was a stray quote and so a parse error.

jmagman
jmagman commented Oct 20, 2022
View reviewed changes
.clang-tidy Show resolved Hide resolved
jmagman
jmagman commented Oct 20, 2022
View reviewed changes
.clang-tidy Show resolved Hide resolved
@jmagman jmagman requested a review from zanderso October 20, 2022 03:59
@jmagman jmagman marked this pull request as ready for review October 20, 2022 03:59
zanderso
zanderso approved these changes Oct 20, 2022
View reviewed changes
.clang-tidy Show resolved Hide resolved
@jmagman jmagman mentioned this pull request Oct 20, 2022
Closed
@gaaclarke gaaclarke mentioned this pull request Oct 21, 2022
Merged
8 tasks
gaaclarke
gaaclarke requested changes Oct 21, 2022
View reviewed changes
Copy link
Member
@gaaclarke gaaclarke left a 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.

@chinmaygarde
Copy link
Member

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.

@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Nov 10, 2022
@gaaclarke
Copy link
Member

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.

@jmagman jmagman mentioned this pull request Nov 17, 2022
Closed
8 tasks
@jmagman
Copy link
Member Author
jmagman commented Nov 17, 2022 •
edited

For example the removal of c style casts should be complete now.

@gaaclarke I still see three on Linux host

❌ Failures for clang-tidy on /b/s/w/ir/cache/builder/src/flutter/flow/layers/backdrop_filter_layer.cc:
/b/s/w/ir/cache/builder/src/flutter/flow/layers/backdrop_filter_layer.cc:45:63: error: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast [google-readability-casting,-warnings-as-errors]
      Layer::AutoPrerollSaveLayerState::Create(context, true, bool(filter_));

❌ Failures for clang-tidy on /b/s/w/ir/cache/builder/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc:
/b/s/w/ir/cache/builder/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc:67:45: error: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast [google-readability-casting,-warnings-as-errors]
    functions[ShaderKey{key_name, stage}] = std::shared_ptr<ShaderFunctionGLES>(

❌ Failures for clang-tidy on /b/s/w/ir/cache/builder/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc:
/b/s/w/ir/cache/builder/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc:88:45: error: C-style casts are discouraged; use static_cast/const_cast/reinterpret_cast [google-readability-casting,-warnings-as-errors]
    functions[ShaderKey{key_name, stage}] = std::shared_ptr<ShaderFunctionVK>(

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8797288801511583249/+/u/test:_lint_host_debug/stdout

(impeller/renderer/backend is building when targeting Linux host?)

Filed flutter/flutter#115518

@jmagman jmagman mentioned this pull request Nov 17, 2022
Open
@jmagman
Copy link
Member Author
jmagman commented Nov 17, 2022

google-build-using-namespace warnings: flutter/flutter#115521
google-readability-avoid-underscore-in-googletest-name warnings: #37700
google-default-arguments: flutter/flutter#115522
google-readability-casting: flutter/flutter#115518

@gaaclarke
Copy link
Member

@gaaclarke I still see three on Linux host

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.

@jmagman
Copy link
Member Author
jmagman commented Nov 17, 2022

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.
Draft at #37698

@jmagman jmagman removed the Work in progress (WIP) Not ready (yet) for review! label Nov 17, 2022
gaaclarke
gaaclarke approved these changes Nov 17, 2022
View reviewed changes
Copy link
Member
@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

lgtm!

@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 4.
View them at https://flutter-engine-gold.skia.org/cl/github/36885

@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 18, 2022
@auto-submit auto-submit bot merged commit 1f4ddc3 into flutter:main Nov 18, 2022
@jmagman jmagman deleted the clang-tidy-errors branch November 18, 2022 00:04
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 18, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 18, 2022
@engine-flutter-autoroll
1f4ddc3ee Only run clang-tidy warning checks reported as errors (flut…
f088798 
…ter/engine#36885)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 18, 2022
@engine-flutter-autoroll
Roll Flutter Engine from 80b25a302b4c to e812122e4060 (2 revisions) (#…
450f162 
…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)
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
@engine-flutter-autoroll @shogohida
Roll Flutter Engine from 80b25a302b4c to e812122e4060 (2 revisions) (f…
4e1993d 
…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)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
@engine-flutter-autoroll @gspencergoog
Roll Flutter Engine from 80b25a302b4c to e812122e4060 (2 revisions) (f…
3137cdb 
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zanderso zanderso zanderso approved these changes

@gaaclarke gaaclarke gaaclarke approved these changes

Assignees

@jmagman jmagman

Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Milestone
No milestone
5 participants
@jmagman @chinmaygarde @gaaclarke @skia-gold @zanderso