Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param` #44936

matanlurey · 2023-08-22T00:32:11Z

Zach, this is a pretty naive attempt, so feel free to suggest alternatives. I considered for example just using --checks=-{{LINT}} as well, but figured that might be less discoverable (especially for non-core folks) than just a file called .clang-tidy-for-githooks.

/cc @jonahwilliams

matanlurey · 2023-08-22T01:37:19Z

Whomever reviews this, I have no idea how to pass the LICENSES check and could use an adult!

tools/clang_tidy/lib/src/options.dart

tools/githooks/lib/src/pre_push_command.dart

zanderso · 2023-08-22T02:30:57Z

.clang-tidy-for-githooks

@@ -0,0 +1,41 @@
+# Keep in sync with .clang-tidy.


We should try to think of some ideas to avoid the duplication. Here's one:

The .clang-tidy file is just a yaml file. If clang-tidy ignores fields it doesn't understand, we could add one in the existing .clang-tidy file, like IgnoredCheckForFlutterGitHook (or some better name). Then when running in the context of the pre-push hook, our clang-tidy wrapper can create a new .clang-tidy file under /tmp, and pass that to the clang-tidy --config-file flag.

Done, though I couldn't quite handle the way we both wanted as I'll need to bring in the YAML package.

OK with doing that, but it should probably be a follow-up PR. Wdyt of my hack?

zanderso · 2023-08-22T02:35:23Z

Whomever reviews this, I have no idea how to pass the LICENSES check and could use an adult!

There are instructions at the bottom of the logs for the failing check: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8772102403923285345/+/u/test:_licenses_check/stdout?format=raw.

zanderso

lgtm w/ temp dir cleanup.

zanderso · 2023-08-22T21:23:03Z

tools/clang_tidy/lib/clang_tidy.dart

@@ -292,6 +299,10 @@ class ClangTidy {
    List<Command> commands,
    Options options,
  ) async {
+    io.File? configPath = options.configPath;
+    if (configPath != null && options.excludeSlowChecks) {
+      configPath = _createClangTidyConfigExcludingSlowLints(configPath);


The temp directory should probably be deleted sometime after _runJobs finishes up on line 171.

…or-githooks

matanlurey · 2023-08-22T21:32:17Z

Done!

auto-submit · 2023-08-22T21:49:25Z

auto label is removed for flutter/engine/44936, due to - The status or check suite Linux linux_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-08-23T00:16:18Z

auto label is removed for flutter/engine/44936, due to - The status or check suite Linux linux_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…nce-unnecessary-value-param` (flutter/engine#44936)

…sions) (#133106) Manual roll requested by [email protected] flutter/engine@b190f90...7d56840 2023-08-23 [email protected] Revert "Roll Dart SDK from ab417bc74bb1 to c162b4979562 (1 revision)" (flutter/engine#44989) 2023-08-23 [email protected] Roll Fuchsia Mac SDK from G25oJMO5jbUi-UN4F... to DoQ8KUxSk-5EU6VQ1... (flutter/engine#44988) 2023-08-23 [email protected] Revert "Make `FontWeight` an enum, Remove unused text classes" (flutter/engine#44987) 2023-08-23 [email protected] Roll Dart SDK from ab417bc74bb1 to c162b4979562 (1 revision) (flutter/engine#44986) 2023-08-23 [email protected] Roll Fuchsia Linux SDK from kKI09su99b0AKs8b3... to VSvpNFoFjqXIQTcs6... (flutter/engine#44984) 2023-08-23 [email protected] Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param` (flutter/engine#44936) 2023-08-22 [email protected] Restore old SurfaceTextureExternal drawing code (flutter/engine#44979) 2023-08-22 [email protected] Roll Skia from d0918de21c1a to aa208c8a2d60 (2 revisions) (flutter/engine#44981) 2023-08-22 [email protected] Initialize the texture destruction callback in the Metal embedder test harness (flutter/engine#44973) 2023-08-22 [email protected] Further filter/clear `<SkPaint>.setDither(true)`, this time in `DlSkPaintDispatchHelper` (flutter/engine#44912) 2023-08-22 [email protected] Roll Dart SDK from 3ebf0fedfceb to ab417bc74bb1 (1 revision) (flutter/engine#44977) 2023-08-22 [email protected] Roll Skia from bf6019be75ef to d0918de21c1a (3 revisions) (flutter/engine#44975) 2023-08-22 [email protected] Roll Skia from c675298ddeda to bf6019be75ef (3 revisions) (flutter/engine#44974) 2023-08-22 [email protected] Make `FontWeight` an enum, Remove unused text classes (flutter/engine#44960) 2023-08-22 [email protected] Roll Skia from 9f4b81aac175 to c675298ddeda (2 revisions) (flutter/engine#44971) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from kKI09su99b0A to VSvpNFoFjqXI fuchsia/sdk/core/mac-amd64 from G25oJMO5jbUi to DoQ8KUxSk-5E If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…nce-unnecessary-value-param` (flutter/engine#44936)

…e-unnecessary-value-param` (#44936)" This reverts commit 4d1050f.

…e-unnecessary-value-param`" (#45020) Reverts #44936

Partial (safe) re-land of #44936.

Partial work towards re-landing #44936. Both the `clang_tidy` and `githooks` passage could benefit from being able to automatically find the latest `compile_commands.json` output, which means that some common code should exist in the `tools/` directory. This is a very minimal (but tested) library for doing exactly that.

@jonahwilliams

…ssary-value-param` (flutter#44936) Closes flutter/flutter#132687. Zach, this is a pretty naive attempt, so feel free to suggest alternatives. I considered for example just using `--checks=-{{LINT}}` as well, but figured that might be less discoverable (especially for non-core folks) than just a file called `.clang-tidy-for-githooks`. /cc @jonahwilliams

…e-unnecessary-value-param`" (flutter#45020) Reverts flutter#44936

Partial (safe) re-land of flutter#44936.

…ter#45154) Partial work towards re-landing flutter#44936. Both the `clang_tidy` and `githooks` passage could benefit from being able to automatically find the latest `compile_commands.json` output, which means that some common code should exist in the `tools/` directory. This is a very minimal (but tested) library for doing exactly that.

matanlurey added 3 commits August 21, 2023 17:19

Augment the clang-tidy pre-push to use a different config.

d0dfeaa

Fix and add test.

f38c264

Whitespace.

7c7d385

matanlurey requested a review from zanderso August 22, 2023 00:32

matanlurey added 2 commits August 21, 2023 17:47

Ignore licenses.

a278a9c

Also update this I suppose.

dce3ced

Merge branch 'main' into engine-clang-tidy-for-githooks

ad5fac4

zanderso reviewed Aug 22, 2023

View reviewed changes

matanlurey added 5 commits August 22, 2023 09:58

Use some suggestions from Zach on clang_tidy resolution.

46d184e

Address comments, accumulate different kinds of tech debt.

2ffa7b3

Format.

30c4ee8

Remove whitespace.

5c3c5af

Fix resolution.

70e2a25

matanlurey requested a review from zanderso August 22, 2023 21:11

zanderso approved these changes Aug 22, 2023

View reviewed changes

matanlurey added 2 commits August 22, 2023 14:31

Delete temp directory.

2becd72

Merge remote-tracking branch 'upstream/main' into engine-clang-tidy-f…

b4bc1e4

…or-githooks

matanlurey force-pushed the engine-clang-tidy-for-githooks branch from a71ee6d to b4bc1e4 Compare August 22, 2023 21:32

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 22, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 22, 2023

Merge branch 'main' into engine-clang-tidy-for-githooks

d7a9f1c

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 22, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2023

Fix test.

bd3e24c

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2023

auto-submit bot merged commit 4d1050f into flutter:main Aug 23, 2023

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from b190f9015049 to 4d1050f5aec9 (10 revisions) flutter/flutter#133099

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

4d1050f5a Enable clang-tidy for pre-push (opt-out), exclude `performa…

1635ceb

…nce-unnecessary-value-param` (flutter/engine#44936)

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from b190f9015049 to ef2580a5bb68 (11 revisions) flutter/flutter#133100

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

4d1050f5a Enable clang-tidy for pre-push (opt-out), exclude `performa…

9ca3148

…nce-unnecessary-value-param` (flutter/engine#44936)

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from b190f9015049 to 41912e7be15a (12 revisions) flutter/flutter#133104

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

4d1050f5a Enable clang-tidy for pre-push (opt-out), exclude `performa…

97a9427

…nce-unnecessary-value-param` (flutter/engine#44936)

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Manual roll Flutter Engine from b190f9015049 to 7d56840865d2 (15 revisions) flutter/flutter#133106

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

4d1050f5a Enable clang-tidy for pre-push (opt-out), exclude `performa…

0ba9512

…nce-unnecessary-value-param` (flutter/engine#44936)

engine-flutter-autoroll mentioned this pull request Aug 23, 2023

Roll Flutter Engine from b190f9015049 to f48bdba1c167 (17 revisions) flutter/flutter#133108

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023

4d1050f5a Enable clang-tidy for pre-push (opt-out), exclude `performa…

707cece

…nce-unnecessary-value-param` (flutter/engine#44936)

bdero added a commit that referenced this pull request Aug 23, 2023

Revert "Enable clang-tidy for pre-push (opt-out), exclude `performanc…

f4600a9

…e-unnecessary-value-param` (#44936)" This reverts commit 4d1050f.

bdero mentioned this pull request Aug 23, 2023

Revert "Enable clang-tidy for pre-push (opt-out), exclude performance-unnecessary-value-param" #45020

Merged

auto-submit bot pushed a commit that referenced this pull request Aug 23, 2023

Revert "Enable clang-tidy for pre-push (opt-out), exclude `performanc…

90d3893

…e-unnecessary-value-param`" (#45020) Reverts #44936

matanlurey deleted the engine-clang-tidy-for-githooks branch August 24, 2023 18:09

reidbaker mentioned this pull request Aug 25, 2023

Update to very new gradle version #45124

Closed

8 tasks

This was referenced Aug 27, 2023

A minimal engine_tools_lib to use for local-repo Dart tooling #45154

Merged

Use YAML >- to avoid strange formatting. #45158

Merged

matanlurey added a commit that referenced this pull request Aug 27, 2023

Use YAML >- to avoid strange formatting. (#45158)

1e29989

Partial (safe) re-land of #44936.

gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023

Revert "Enable clang-tidy for pre-push (opt-out), exclude `performanc…

93b0546

…e-unnecessary-value-param`" (flutter#45020) Reverts flutter#44936

gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023

Use YAML >- to avoid strange formatting. (flutter#45158)

27fcdbe

Partial (safe) re-land of flutter#44936.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param` #44936

Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param` #44936

matanlurey commented Aug 22, 2023

matanlurey commented Aug 22, 2023

zanderso Aug 22, 2023

matanlurey Aug 22, 2023 •

edited

Loading

zanderso commented Aug 22, 2023

zanderso left a comment

zanderso Aug 22, 2023

matanlurey commented Aug 22, 2023

auto-submit bot commented Aug 22, 2023

auto-submit bot commented Aug 23, 2023

Enable clang-tidy for pre-push (opt-out), exclude performance-unnecessary-value-param #44936

Enable clang-tidy for pre-push (opt-out), exclude performance-unnecessary-value-param #44936

Conversation

matanlurey commented Aug 22, 2023

matanlurey commented Aug 22, 2023

zanderso Aug 22, 2023

Choose a reason for hiding this comment

matanlurey Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

zanderso commented Aug 22, 2023

zanderso left a comment

Choose a reason for hiding this comment

zanderso Aug 22, 2023

Choose a reason for hiding this comment

matanlurey commented Aug 22, 2023

auto-submit bot commented Aug 22, 2023

auto-submit bot commented Aug 23, 2023

Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param` #44936

Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param` #44936

matanlurey Aug 22, 2023 •

edited

Loading