From 738977664d93cd6639146ed52afd6732f8baf9f8 Mon Sep 17 00:00:00 2001 From: Jenn Magder Date: Tue, 9 Nov 2021 21:35:46 -0800 Subject: [PATCH 1/3] Speed up clang-tidy script by skipping missing files --- tools/clang_tidy/lib/clang_tidy.dart | 32 ++++++++++++---------- tools/clang_tidy/lib/src/command.dart | 15 ++++++---- tools/clang_tidy/test/clang_tidy_test.dart | 18 +++++++++--- 3 files changed, 40 insertions(+), 25 deletions(-) diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index 2cdd81d8ec144..597d782658140 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -110,7 +110,7 @@ class ClangTidy { final List buildCommandsData = jsonDecode( options.buildCommandsPath.readAsStringSync(), ) as List; - final List changedFileBuildCommands = getLintCommandsForChangedFiles( + final List changedFileBuildCommands = await getLintCommandsForChangedFiles( buildCommandsData, changedFiles, ); @@ -165,20 +165,21 @@ class ClangTidy { /// Given a build commands json file, and the files with local changes, /// compute the lint commands to run. @visibleForTesting - List getLintCommandsForChangedFiles( + Future> getLintCommandsForChangedFiles( List buildCommandsData, List changedFiles, - ) { - final List buildCommands = [ - for (final dynamic c in buildCommandsData) - Command.fromMap(c as Map), - ]; - - return [ - for (final Command c in buildCommands) - if (c.containsAny(changedFiles)) - c, - ]; + ) async { + final List buildCommands = []; + for (final dynamic data in buildCommandsData) { + final Command command = Command.fromMap(data as Map); + final LintAction lintAction = await command.lintAction; + if (lintAction != LintAction.skipMissing && + lintAction != LintAction.skipThirdParty && + command.containsAny(changedFiles)) { + buildCommands.add(command); + } + } + return buildCommands; } Future<_ComputeJobsResult> _computeJobs( @@ -212,6 +213,9 @@ class ClangTidy { case LintAction.skipThirdParty: _outSink.writeln('🔷 ignoring $relativePath (third_party)'); break; + case LintAction.skipMissing: + _outSink.writeln('🔷 ignoring $relativePath (missing)'); + break; } } return _ComputeJobsResult(jobs, sawMalformed); @@ -239,5 +243,3 @@ class ClangTidy { return result; } } - - diff --git a/tools/clang_tidy/lib/src/command.dart b/tools/clang_tidy/lib/src/command.dart index 73cbca7f40dcb..d2566ee769754 100644 --- a/tools/clang_tidy/lib/src/command.dart +++ b/tools/clang_tidy/lib/src/command.dart @@ -26,6 +26,9 @@ enum LintAction { /// Fail due to a malformed FLUTTER_NOLINT comment. failMalformedNoLint, + + /// Ignore because the file doesn't exist locally. + skipMissing, } /// A compilation command and methods to generate the lint command and job for @@ -86,20 +89,20 @@ class Command { r'//\s*FLUTTER_NOLINT(: https://github.com/flutter/flutter/issues/\d+)?', ); - LintAction? _lintAction; - /// The type of lint that is appropriate for this command. - Future get lintAction async => - _lintAction ??= await getLintAction(filePath); + late final Future lintAction = getLintAction(filePath); /// Determine the lint action for the file at `path`. @visibleForTesting - static Future getLintAction(String filePath) { + static Future getLintAction(String filePath) async { if (path.split(filePath).contains('third_party')) { - return Future.value(LintAction.skipThirdParty); + return LintAction.skipThirdParty; } final io.File file = io.File(filePath); + if (!file.existsSync()) { + return LintAction.skipMissing; + } final Stream lines = file.openRead() .transform(utf8.decoder) .transform(const LineSplitter()); diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart index 9ca27010c4732..0cbe76883ba42 100644 --- a/tools/clang_tidy/test/clang_tidy_test.dart +++ b/tools/clang_tidy/test/clang_tidy_test.dart @@ -106,7 +106,7 @@ Future main(List args) async { [ '--compile-commands', // This just has to exist. - io.Platform.executable, + io.Platform.script.path, '--repo', '/does/not/exist', ], @@ -168,7 +168,7 @@ Future main(List args) async { 'file': filePath, }, ]; - final List commands = clangTidy.getLintCommandsForChangedFiles( + final List commands = await clangTidy.getLintCommandsForChangedFiles( buildCommandsData, [], ); @@ -186,7 +186,9 @@ Future main(List args) async { outSink: outBuffer, errSink: errBuffer, ); - const String filePath = '/path/to/a/source_file.cc'; + + // This just has to exist. + final String filePath = io.Platform.script.path; final List buildCommandsData = >[ { 'directory': '/unused', @@ -194,7 +196,7 @@ Future main(List args) async { 'file': filePath, }, ]; - final List commands = clangTidy.getLintCommandsForChangedFiles( + final List commands = await clangTidy.getLintCommandsForChangedFiles( buildCommandsData, [io.File(filePath)], ); @@ -220,6 +222,14 @@ Future main(List args) async { expect(lintAction, equals(LintAction.skipThirdParty)); }); + test('Command getLintAction flags missing files', () async { + final LintAction lintAction = await Command.getLintAction( + 'does/not/exist', + ); + + expect(lintAction, equals(LintAction.skipMissing)); + }); + test('Command getLintActionFromContents flags FLUTTER_NOLINT', () async { final LintAction lintAction = await Command.lintActionFromContents( Stream.fromIterable([ From bb02b88dac629f5e3d483b2f34c19ddd8066755a Mon Sep 17 00:00:00 2001 From: Jenn Magder Date: Wed, 10 Nov 2021 00:12:11 -0800 Subject: [PATCH 2/3] Add comment for why third_party is being skipped --- tools/clang_tidy/lib/clang_tidy.dart | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/clang_tidy/lib/clang_tidy.dart b/tools/clang_tidy/lib/clang_tidy.dart index 597d782658140..67599166046b9 100644 --- a/tools/clang_tidy/lib/clang_tidy.dart +++ b/tools/clang_tidy/lib/clang_tidy.dart @@ -173,9 +173,8 @@ class ClangTidy { for (final dynamic data in buildCommandsData) { final Command command = Command.fromMap(data as Map); final LintAction lintAction = await command.lintAction; - if (lintAction != LintAction.skipMissing && - lintAction != LintAction.skipThirdParty && - command.containsAny(changedFiles)) { + // Short-circuit the expensive containsAny call for the many third_party files. + if (lintAction != LintAction.skipThirdParty && command.containsAny(changedFiles)) { buildCommands.add(command); } } From e738546e4f6175bc2e14d5f5a2e3f506482a3ef5 Mon Sep 17 00:00:00 2001 From: Jenn Magder Date: Wed, 10 Nov 2021 10:09:21 -0800 Subject: [PATCH 3/3] Update test comment --- tools/clang_tidy/test/clang_tidy_test.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/clang_tidy/test/clang_tidy_test.dart b/tools/clang_tidy/test/clang_tidy_test.dart index 0cbe76883ba42..4656f33926e98 100644 --- a/tools/clang_tidy/test/clang_tidy_test.dart +++ b/tools/clang_tidy/test/clang_tidy_test.dart @@ -105,7 +105,7 @@ Future main(List args) async { final ClangTidy clangTidy = ClangTidy.fromCommandLine( [ '--compile-commands', - // This just has to exist. + // This file needs to exist, and be UTF8 line-parsable. io.Platform.script.path, '--repo', '/does/not/exist', @@ -187,7 +187,7 @@ Future main(List args) async { errSink: errBuffer, ); - // This just has to exist. + // This file needs to exist, and be UTF8 line-parsable. final String filePath = io.Platform.script.path; final List buildCommandsData = >[ { @@ -224,7 +224,7 @@ Future main(List args) async { test('Command getLintAction flags missing files', () async { final LintAction lintAction = await Command.getLintAction( - 'does/not/exist', + '/does/not/exist', ); expect(lintAction, equals(LintAction.skipMissing));