From 340a69147a9f1c6eea6adafeb4930e9dbe79395b Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Dec 2023 13:46:43 -0800 Subject: [PATCH 01/26] Initial commit: directory for header_guard_check. --- tools/header_guard_check/pubspec.yaml | 59 +++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 tools/header_guard_check/pubspec.yaml diff --git a/tools/header_guard_check/pubspec.yaml b/tools/header_guard_check/pubspec.yaml new file mode 100644 index 0000000000000..4f26b7c9771df --- /dev/null +++ b/tools/header_guard_check/pubspec.yaml @@ -0,0 +1,59 @@ +# Copyright 2013 The Flutter Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +name: header_guard_check +publish_to: none + +# Do not add any dependencies that require more than what is provided in +# //third_party/dart/pkg or //third_party/dart/third_party/pkg. +# In particular, package:test is not usable here. + +# If you do add packages here, make sure you can run `pub get --offline`, and +# check the .packages and .package_config to make sure all the paths are +# relative to this directory into //third_party/dart + +environment: + sdk: '>=3.2.0-0 <4.0.0' + +dependencies: + args: any + git_repo_tools: any + engine_repo_tools: any + meta: any + path: any + +dev_dependencies: + async_helper: any + expect: any + litetest: any + process_fakes: any + smith: any + +dependency_overrides: + args: + path: ../../../third_party/dart/third_party/pkg/args + async_helper: + path: ../../../third_party/dart/pkg/async_helper + engine_repo_tools: + path: ../pkg/engine_repo_tools + expect: + path: ../../../third_party/dart/pkg/expect + file: + path: ../../../third_party/dart/third_party/pkg/file/packages/file + git_repo_tools: + path: ../pkg/git_repo_tools + litetest: + path: ../../testing/litetest + path: + path: ../../../third_party/dart/third_party/pkg/path + platform: + path: ../../third_party/pkg/platform + process: + path: ../../third_party/pkg/process + process_fakes: + path: ../pkg/process_fakes + process_runner: + path: ../../third_party/pkg/process_runner + smith: + path: ../../../third_party/dart/pkg/smith From 0058f60f9f7da6eec63d6949e39b6bedb13d1940 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Dec 2023 14:40:05 -0800 Subject: [PATCH 02/26] WIP. --- tools/header_guard_check/lib/src/header_file.dart | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 tools/header_guard_check/lib/src/header_file.dart diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart new file mode 100644 index 0000000000000..6d2d8d88b72d0 --- /dev/null +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -0,0 +1,15 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:meta/meta.dart'; +import 'package:path/path.dart' as p; + +/// Represents a C++ header file, i.e. a file on disk that ends in `.h`. +@immutable +final class HeaderFile { + /// Path to the file on disk. + final String path; + + const HeaderFile(this.path); +} From 3aed665aa44fb766be7d7f83acecc99681df98d1 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Dec 2023 15:32:37 -0800 Subject: [PATCH 03/26] Add minimal library with some validation. --- .../lib/src/header_file.dart | 215 ++++++++++++++++- tools/header_guard_check/pubspec.yaml | 7 + .../test/header_file_test.dart | 217 ++++++++++++++++++ 3 files changed, 438 insertions(+), 1 deletion(-) create mode 100644 tools/header_guard_check/test/header_file_test.dart diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index 6d2d8d88b72d0..d6f01de3b8c87 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -2,14 +2,227 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:io' as io; + import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; +import 'package:source_span/source_span.dart'; /// Represents a C++ header file, i.e. a file on disk that ends in `.h`. @immutable final class HeaderFile { + /// Creates a new header file from the given [path]. + const HeaderFile.from( + this.path, { + required this.guard, + required this.pragmaOnce, + }); + + /// Parses the given [path] as a header file. + /// + /// Throws an [ArgumentError] if the file does not exist. + factory HeaderFile.parse(String path) { + final io.File file = io.File(path); + if (!file.existsSync()) { + throw ArgumentError.value(path, 'path', 'File does not exist.'); + } + + final String contents = file.readAsStringSync(); + final SourceFile sourceFile = SourceFile.fromString(contents, url: p.toUri(path)); + return HeaderFile.from( + path, + guard: _parseGuard(sourceFile), + pragmaOnce: _parsePragmaOnce(sourceFile), + ); + } + + /// Parses the header guard of the given [sourceFile]. + static HeaderGuardSpans? _parseGuard(SourceFile sourceFile) { + SourceSpan? ifndefSpan; + SourceSpan? defineSpan; + SourceSpan? endifSpan; + + // Iterate over the lines in the file. + for (int i = 0; i < sourceFile.lines; i++) { + final int start = sourceFile.getOffset(i); + final int end = i == sourceFile.lines - 1 + ? sourceFile.length + : sourceFile.getOffset(i + 1) - 1; + final String line = sourceFile.getText(start, end); + + // Check if the line is a header guard directive. + if (line.startsWith('#ifndef')) { + ifndefSpan = sourceFile.span(start, end); + } else if (line.startsWith('#define')) { + defineSpan = sourceFile.span(start, end); + break; + } + } + + // If we found no header guard, return null. + if (ifndefSpan == null) { + return null; + } + + // Now iterate backwards to find the (last) #endif directive. + for (int i = sourceFile.lines - 1; i > 0; i--) { + final int start = sourceFile.getOffset(i); + final int end = i == sourceFile.lines - 1 ? + sourceFile.length : + sourceFile.getOffset(i + 1) - 1; + final String line = sourceFile.getText(start, end); + + // Check if the line is a header guard directive. + if (line.startsWith('#endif')) { + endifSpan = sourceFile.span(start, end); + break; + } + } + + return HeaderGuardSpans( + ifndefSpan: ifndefSpan, + defineSpan: defineSpan, + endifSpan: endifSpan, + ); + } + + /// Parses the `#pragma once` directive of the given [sourceFile]. + static SourceSpan? _parsePragmaOnce(SourceFile sourceFile) { + // Iterate over the lines in the file. + for (int i = 0; i < sourceFile.lines; i++) { + final int start = sourceFile.getOffset(i); + final int end = i == sourceFile.lines - 1 + ? sourceFile.length + : sourceFile.getOffset(i + 1) - 1; + final String line = sourceFile.getText(start, end); + + // Check if the line is a header guard directive. + if (line.startsWith('#pragma once')) { + return sourceFile.span(start, end); + } + } + + return null; + } + /// Path to the file on disk. final String path; - const HeaderFile(this.path); + /// The header guard span, if any. + /// + /// This is `null` if the file does not have a header guard. + final HeaderGuardSpans? guard; + + /// The `#pragma once` directive, if any. + /// + /// This is `null` if the file does not have a `#pragma once` directive. + final SourceSpan? pragmaOnce; + + @override + bool operator ==(Object other) { + return other is HeaderFile && + path == other.path && + guard == other.guard && + pragmaOnce == other.pragmaOnce; + } + + @override + int get hashCode => Object.hash(path, guard, pragmaOnce); + + @override + String toString() { + return 'HeaderFile(\n' + ' path: $path\n' + ' guard: $guard\n' + ' pragmaOnce: $pragmaOnce\n' + ')'; + } +} + +/// Source elements that are part of a header guard. +@immutable +final class HeaderGuardSpans { + /// Collects the source spans of the header guard directives. + const HeaderGuardSpans({ + required this.ifndefSpan, + required this.defineSpan, + required this.endifSpan, + }); + + /// Location of the `#ifndef` directive. + final SourceSpan? ifndefSpan; + + /// Location of the `#define` directive. + final SourceSpan? defineSpan; + + /// Location of the `#endif` directive. + final SourceSpan? endifSpan; + + @override + bool operator ==(Object other) { + return other is HeaderGuardSpans && + ifndefSpan == other.ifndefSpan && + defineSpan == other.defineSpan && + endifSpan == other.endifSpan; + } + + @override + int get hashCode => Object.hash(ifndefSpan, defineSpan, endifSpan); + + @override + String toString() { + return 'HeaderGuardSpans(\n' + ' #ifndef: $ifndefSpan\n' + ' #define: $defineSpan\n' + ' #endif: $endifSpan\n' + ')'; + } + + /// Returns the value of the `#ifndef` directive. + /// + /// For example, `#ifndef FOO_H_`, this will return `FOO_H_`. + /// + /// If the span is not a valid `#ifndef` directive, `null` is returned. + String? get ifndefValue { + final String? value = ifndefSpan?.text; + if (value == null) { + return null; + } + if (!value.startsWith('#ifndef ')) { + return null; + } + return value.substring('#ifndef '.length); + } + + /// Returns the value of the `#define` directive. + /// + /// For example, `#define FOO_H_`, this will return `FOO_H_`. + /// + /// If the span is not a valid `#define` directive, `null` is returned. + String? get defineValue { + final String? value = defineSpan?.text; + if (value == null) { + return null; + } + if (!value.startsWith('#define ')) { + return null; + } + return value.substring('#define '.length); + } + + /// Returns the value of the `#endif` directive. + /// + /// For example, `#endif // FOO_H_`, this will return `FOO_H_`. + /// + /// If the span is not a valid `#endif` directive, `null` is returned. + String? get endifValue { + final String? value = endifSpan?.text; + if (value == null) { + return null; + } + if (!value.startsWith('#endif // ')) { + return null; + } + return value.substring('#endif // '.length); + } } diff --git a/tools/header_guard_check/pubspec.yaml b/tools/header_guard_check/pubspec.yaml index 4f26b7c9771df..64e676d6b17fd 100644 --- a/tools/header_guard_check/pubspec.yaml +++ b/tools/header_guard_check/pubspec.yaml @@ -22,6 +22,7 @@ dependencies: engine_repo_tools: any meta: any path: any + source_span: any dev_dependencies: async_helper: any @@ -35,6 +36,8 @@ dependency_overrides: path: ../../../third_party/dart/third_party/pkg/args async_helper: path: ../../../third_party/dart/pkg/async_helper + collection: + path: ../../../third_party/dart/third_party/pkg/collection engine_repo_tools: path: ../pkg/engine_repo_tools expect: @@ -57,3 +60,7 @@ dependency_overrides: path: ../../third_party/pkg/process_runner smith: path: ../../../third_party/dart/pkg/smith + source_span: + path: ../../../third_party/dart/third_party/pkg/source_span + term_glyph: + path: ../../../third_party/dart/third_party/pkg/term_glyph diff --git a/tools/header_guard_check/test/header_file_test.dart b/tools/header_guard_check/test/header_file_test.dart new file mode 100644 index 0000000000000..a8abd41c02475 --- /dev/null +++ b/tools/header_guard_check/test/header_file_test.dart @@ -0,0 +1,217 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' as io; + +import 'package:header_guard_check/src/header_file.dart'; +import 'package:litetest/litetest.dart'; +import 'package:path/path.dart' as p; +import 'package:source_span/source_span.dart'; + +Future main(List args) async { + void withTestFile(String path, String contents, void Function(io.File) fn) { + // Create a temporary file and delete it when we're done. + final io.Directory tempDir = io.Directory.systemTemp.createTempSync('header_guard_check_test'); + final io.File file = io.File(p.join(tempDir.path, path)); + file.writeAsStringSync(contents); + try { + fn(file); + } finally { + tempDir.deleteSync(recursive: true); + } + } + + group('HeaderGuardSpans', () { + test('parses #ifndef', () { + const String input = '#ifndef FOO_H_'; + final HeaderGuardSpans guard = HeaderGuardSpans( + ifndefSpan: SourceSpanWithContext( + SourceLocation(0), + SourceLocation(input.length), + input, + input, + ), + defineSpan: null, + endifSpan: null, + ); + expect(guard.ifndefValue, 'FOO_H_'); + }); + + test('ignores #ifndef if omitted', () { + const HeaderGuardSpans guard = HeaderGuardSpans( + ifndefSpan: null, + defineSpan: null, + endifSpan: null, + ); + expect(guard.ifndefValue, isNull); + }); + + test('ignores #ifndef if invalid', () { + const String input = '#oops FOO_H_'; + final HeaderGuardSpans guard = HeaderGuardSpans( + ifndefSpan: SourceSpanWithContext( + SourceLocation(0), + SourceLocation(input.length), + input, + input, + ), + defineSpan: null, + endifSpan: null, + ); + expect(guard.ifndefValue, isNull); + }); + + test('parses #define', () { + const String input = '#define FOO_H_'; + final HeaderGuardSpans guard = HeaderGuardSpans( + ifndefSpan: null, + defineSpan: SourceSpanWithContext( + SourceLocation(0), + SourceLocation(input.length), + input, + input, + ), + endifSpan: null, + ); + expect(guard.defineValue, 'FOO_H_'); + }); + + test('ignores #define if omitted', () { + const HeaderGuardSpans guard = HeaderGuardSpans( + ifndefSpan: null, + defineSpan: null, + endifSpan: null, + ); + expect(guard.defineValue, isNull); + }); + + test('ignores #define if invalid', () { + const String input = '#oops FOO_H_'; + final HeaderGuardSpans guard = HeaderGuardSpans( + ifndefSpan: null, + defineSpan: SourceSpanWithContext( + SourceLocation(0), + SourceLocation(input.length), + input, + input, + ), + endifSpan: null, + ); + expect(guard.defineValue, isNull); + }); + + test('parses #endif', () { + const String input = '#endif // FOO_H_'; + final HeaderGuardSpans guard = HeaderGuardSpans( + ifndefSpan: null, + defineSpan: null, + endifSpan: SourceSpanWithContext( + SourceLocation(0), + SourceLocation(input.length), + input, + input, + ), + ); + expect(guard.endifValue, 'FOO_H_'); + }); + + test('ignores #endif if omitted', () { + const HeaderGuardSpans guard = HeaderGuardSpans( + ifndefSpan: null, + defineSpan: null, + endifSpan: null, + ); + expect(guard.endifValue, isNull); + }); + + test('ignores #endif if invalid', () { + const String input = '#oops // FOO_H_'; + final HeaderGuardSpans guard = HeaderGuardSpans( + ifndefSpan: null, + defineSpan: null, + endifSpan: SourceSpanWithContext( + SourceLocation(0), + SourceLocation(input.length), + input, + input, + ), + ); + expect(guard.endifValue, isNull); + }); + }); + + group('HeaderFile', () { + test('parses a header file with a valid guard', () { + final String input = [ + '#ifndef FOO_H_', + '#define FOO_H_', + '', + '#endif // FOO_H_', + ].join('\n'); + withTestFile('foo.h', input, (io.File file) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + expect(headerFile.guard!.ifndefValue, 'FOO_H_'); + expect(headerFile.guard!.defineValue, 'FOO_H_'); + expect(headerFile.guard!.endifValue, 'FOO_H_'); + }); + }); + + test('parses a header file with an invalid #endif', () { + final String input = [ + '#ifndef FOO_H_', + '#define FOO_H_', + '', + // No comment after the #endif. + '#endif', + ].join('\n'); + withTestFile('foo.h', input, (io.File file) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + expect(headerFile.guard!.ifndefValue, 'FOO_H_'); + expect(headerFile.guard!.defineValue, 'FOO_H_'); + expect(headerFile.guard!.endifValue, isNull); + }); + }); + + test('parses a header file with a missing #define', () { + final String input = [ + '#ifndef FOO_H_', + // No #define. + '', + '#endif // FOO_H_', + ].join('\n'); + withTestFile('foo.h', input, (io.File file) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + expect(headerFile.guard!.ifndefValue, 'FOO_H_'); + expect(headerFile.guard!.defineValue, isNull); + expect(headerFile.guard!.endifValue, 'FOO_H_'); + }); + }); + + test('parses a header file with a missing #ifndef', () { + final String input = [ + // No #ifndef. + '#define FOO_H_', + '', + '#endif // FOO_H_', + ].join('\n'); + withTestFile('foo.h', input, (io.File file) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + expect(headerFile.guard, isNull); + }); + }); + + test('parses a header file with a #pragma once', () { + final String input = [ + '#pragma once', + '', + ].join('\n'); + withTestFile('foo.h', input, (io.File file) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + expect(headerFile.pragmaOnce, isNotNull); + }); + }); + }); + + return 0; +} From 1784e68a0b0c3b9541a708bc15e172e54b5e5156 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Dec 2023 16:04:16 -0800 Subject: [PATCH 04/26] Working basic tool. --- tools/header_guard_check/README.md | 27 ++++ tools/header_guard_check/bin/main.dart | 15 ++ .../lib/header_guard_check.dart | 128 ++++++++++++++++++ 3 files changed, 170 insertions(+) create mode 100644 tools/header_guard_check/README.md create mode 100644 tools/header_guard_check/bin/main.dart create mode 100644 tools/header_guard_check/lib/header_guard_check.dart diff --git a/tools/header_guard_check/README.md b/tools/header_guard_check/README.md new file mode 100644 index 0000000000000..e0b7333458570 --- /dev/null +++ b/tools/header_guard_check/README.md @@ -0,0 +1,27 @@ +# header_guard_check + +A tool to check that C++ header guards are used consistently in the engine. + +```shell +# Assuming you are in the `flutter` root of the engine repo. +dart ./tools/header_guard_check/bin/main.dart +``` + +The tool checks _all_ header files for the following pattern: + +```h +// path/to/file.h + +#ifndef PATH_TO_FILE_H_ +#define PATH_TO_FILE_H_ +... +#endif // PATH_TO_FILE_H_ +``` + +If the header file does not follow this pattern, the tool will print an error +message and exit with a non-zero exit code. For more information about why we +use this pattern, see [the Google C++ style guide](https://google.github.io/styleguide/cppguide.html#The__define_Guard). + +> [!IMPORTANT] +> This is a prototype tool and is not yet integrated into the engine's CI, nor +> provides a way to fix the header guards automatically. diff --git a/tools/header_guard_check/bin/main.dart b/tools/header_guard_check/bin/main.dart new file mode 100644 index 0000000000000..014b51c2d839d --- /dev/null +++ b/tools/header_guard_check/bin/main.dart @@ -0,0 +1,15 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' as io; + +import 'package:header_guard_check/header_guard_check.dart'; + +Future main(List arguments) async { + final int result = await HeaderGuardCheck.fromCommandLine(arguments).run(); + if (result != 0) { + io.exit(result); + } + return result; +} diff --git a/tools/header_guard_check/lib/header_guard_check.dart b/tools/header_guard_check/lib/header_guard_check.dart new file mode 100644 index 0000000000000..5fb6e8ee1922b --- /dev/null +++ b/tools/header_guard_check/lib/header_guard_check.dart @@ -0,0 +1,128 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' as io; + +import 'package:args/args.dart'; +import 'package:engine_repo_tools/engine_repo_tools.dart'; +import 'package:meta/meta.dart'; +import 'package:path/path.dart' as p; + +import 'src/header_file.dart'; + +/// Checks C++ header files for header guards. +@immutable +final class HeaderGuardCheck { + /// Creates a new header guard checker. + const HeaderGuardCheck({ + required this.source, + required this.exclude, + }); + + /// Parses the command line arguments and creates a new header guard checker. + factory HeaderGuardCheck.fromCommandLine(List arguments) { + final ArgResults argResults = _parser.parse(arguments); + return HeaderGuardCheck( + source: Engine.fromSrcPath(argResults['root'] as String), + exclude: argResults['exclude'] as List, + ); + } + + /// Engine source root. + final Engine source; + + /// Path directories to exclude from the check. + final List exclude; + + /// Runs the header guard check. + Future run() async { + final List files = []; + + // Recursive search for header files. + final io.Directory dir = source.flutterDir; + await for (final io.FileSystemEntity entity in dir.list(recursive: true)) { + if (entity is io.File && entity.path.endsWith('.h')) { + // Check that the file is not excluded. + bool excluded = false; + for (final String excludePath in exclude) { + if (p.isWithin(excludePath, entity.path)) { + excluded = true; + break; + } + } + if (!excluded) { + files.add(entity); + } + } + } + + // Check each file. + final List badFiles = []; + for (final io.File file in files) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + if (headerFile.pragmaOnce != null) { + io.stderr.writeln(headerFile.pragmaOnce!.message('Unexpected #pragma once')); + badFiles.add(headerFile); + continue; + } + if (headerFile.guard == null) { + io.stderr.writeln('Missing header guard in ${headerFile.path}'); + badFiles.add(headerFile); + continue; + } + + // Determine what the expected guard should be. + // Find the relative path from the engine root to the file. + final String relativePath = p.relative(file.path, from: source.flutterDir.path); + final String underscoredRelativePath = relativePath.replaceAll(p.separator, '_'); + final String expectedGuard = 'FLUTTER_${p.withoutExtension(underscoredRelativePath).toUpperCase().replaceAll('.', '_')}_H_'; + if (headerFile.guard!.ifndefValue != expectedGuard) { + io.stderr.writeln(headerFile.guard!.ifndefSpan!.message('Expected #ifndef $expectedGuard')); + badFiles.add(headerFile); + continue; + } + if (headerFile.guard!.defineValue != expectedGuard) { + io.stderr.writeln(headerFile.guard!.defineSpan!.message('Expected #define $expectedGuard')); + badFiles.add(headerFile); + continue; + } + if (headerFile.guard!.endifValue != expectedGuard) { + io.stderr.writeln(headerFile.guard!.endifSpan!.message('Expected #endif // $expectedGuard')); + badFiles.add(headerFile); + continue; + } + } + + if (badFiles.isNotEmpty) { + io.stdout.writeln('The following ${badFiles.length} files have invalid header guards:'); + for (final HeaderFile headerFile in badFiles) { + io.stdout.writeln(' ${headerFile.path}'); + } + return 1; + } + + return 0; + } +} + +final Engine? _engine = Engine.tryFindWithin(p.dirname(p.fromUri(io.Platform.script))); + +final ArgParser _parser = ArgParser() + ..addOption( + 'root', + abbr: 'r', + help: 'Path to the engine source root.', + valueHelp: 'path/to/engine/src', + defaultsTo: _engine?.srcDir.path, + ) + ..addMultiOption( + 'exclude', + abbr: 'e', + help: 'Path directories to exclude from the check.', + valueHelp: 'path/to/dir', + defaultsTo: _engine != null ? [ + p.join(_engine!.flutterDir.path, 'build'), + p.join(_engine!.flutterDir.path, 'third_party'), + ] : null, + ); From 90892fad66b6437485cea5d7d307c8072d3b424c Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Dec 2023 16:06:15 -0800 Subject: [PATCH 05/26] WS. --- tools/header_guard_check/lib/header_guard_check.dart | 4 ++-- tools/header_guard_check/lib/src/header_file.dart | 2 +- tools/header_guard_check/pubspec.yaml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/header_guard_check/lib/header_guard_check.dart b/tools/header_guard_check/lib/header_guard_check.dart index 5fb6e8ee1922b..62d1ba1dac203 100644 --- a/tools/header_guard_check/lib/header_guard_check.dart +++ b/tools/header_guard_check/lib/header_guard_check.dart @@ -38,8 +38,8 @@ final class HeaderGuardCheck { /// Runs the header guard check. Future run() async { final List files = []; - - // Recursive search for header files. + + // Recursive search for header files. final io.Directory dir = source.flutterDir; await for (final io.FileSystemEntity entity in dir.list(recursive: true)) { if (entity is io.File && entity.path.endsWith('.h')) { diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index d6f01de3b8c87..d5da4a9db2de7 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -67,7 +67,7 @@ final class HeaderFile { // Now iterate backwards to find the (last) #endif directive. for (int i = sourceFile.lines - 1; i > 0; i--) { final int start = sourceFile.getOffset(i); - final int end = i == sourceFile.lines - 1 ? + final int end = i == sourceFile.lines - 1 ? sourceFile.length : sourceFile.getOffset(i + 1) - 1; final String line = sourceFile.getText(start, end); diff --git a/tools/header_guard_check/pubspec.yaml b/tools/header_guard_check/pubspec.yaml index 64e676d6b17fd..2796069a76580 100644 --- a/tools/header_guard_check/pubspec.yaml +++ b/tools/header_guard_check/pubspec.yaml @@ -16,7 +16,7 @@ publish_to: none environment: sdk: '>=3.2.0-0 <4.0.0' -dependencies: +dependencies: args: any git_repo_tools: any engine_repo_tools: any From 5b21cc1b316d9e022c1bf8564a5759ac9317c6d4 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 11 Dec 2023 16:10:21 -0800 Subject: [PATCH 06/26] Tweak. --- tools/header_guard_check/lib/header_guard_check.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/header_guard_check/lib/header_guard_check.dart b/tools/header_guard_check/lib/header_guard_check.dart index 62d1ba1dac203..05057bf0673c7 100644 --- a/tools/header_guard_check/lib/header_guard_check.dart +++ b/tools/header_guard_check/lib/header_guard_check.dart @@ -123,6 +123,7 @@ final ArgParser _parser = ArgParser() valueHelp: 'path/to/dir', defaultsTo: _engine != null ? [ p.join(_engine!.flutterDir.path, 'build'), + p.join(_engine!.flutterDir.path, 'prebuilts'), p.join(_engine!.flutterDir.path, 'third_party'), ] : null, ); From 98e3307ccd8ca25be2364916f149d52431bab394 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Dec 2023 17:06:06 -0800 Subject: [PATCH 07/26] Tweak. --- .../lib/header_guard_check.dart | 62 +++++++++++++------ 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/tools/header_guard_check/lib/header_guard_check.dart b/tools/header_guard_check/lib/header_guard_check.dart index 05057bf0673c7..32c942ddb8b95 100644 --- a/tools/header_guard_check/lib/header_guard_check.dart +++ b/tools/header_guard_check/lib/header_guard_check.dart @@ -18,6 +18,7 @@ final class HeaderGuardCheck { const HeaderGuardCheck({ required this.source, required this.exclude, + this.include = const [], }); /// Parses the command line arguments and creates a new header guard checker. @@ -25,6 +26,7 @@ final class HeaderGuardCheck { final ArgResults argResults = _parser.parse(arguments); return HeaderGuardCheck( source: Engine.fromSrcPath(argResults['root'] as String), + include: argResults['include'] as List, exclude: argResults['exclude'] as List, ); } @@ -32,6 +34,9 @@ final class HeaderGuardCheck { /// Engine source root. final Engine source; + /// Path directories to include in the check. + final List include; + /// Path directories to exclude from the check. final List exclude; @@ -39,23 +44,37 @@ final class HeaderGuardCheck { Future run() async { final List files = []; - // Recursive search for header files. - final io.Directory dir = source.flutterDir; - await for (final io.FileSystemEntity entity in dir.list(recursive: true)) { - if (entity is io.File && entity.path.endsWith('.h')) { - // Check that the file is not excluded. - bool excluded = false; - for (final String excludePath in exclude) { - if (p.isWithin(excludePath, entity.path)) { - excluded = true; - break; + // Recursive search for header files. + final io.Directory dir = source.flutterDir; + await for (final io.FileSystemEntity entity in dir.list(recursive: true)) { + if (entity is io.File && entity.path.endsWith('.h')) { + // Check that the file is included. + bool included = include.isEmpty; + for (final String includePath in include) { + final String relativePath = p.relative(includePath, from: source.flutterDir.path); + if (p.isWithin(relativePath, entity.path)) { + included = true; + break; + } + } + if (!included) { + continue; + } + + // Check that the file is not excluded. + bool excluded = false; + for (final String excludePath in exclude) { + final String relativePath = p.relative(excludePath, from: source.flutterDir.path); + if (p.isWithin(relativePath, entity.path)) { + excluded = true; + break; + } + } + if (!excluded) { + files.add(entity); } - } - if (!excluded) { - files.add(entity); } } - } // Check each file. final List badFiles = []; @@ -116,14 +135,21 @@ final ArgParser _parser = ArgParser() valueHelp: 'path/to/engine/src', defaultsTo: _engine?.srcDir.path, ) + ..addMultiOption( + 'include', + abbr: 'i', + help: 'Path directories to include in the check.', + valueHelp: 'path/to/dir (relative to the engine root)', + defaultsTo: [], + ) ..addMultiOption( 'exclude', abbr: 'e', help: 'Path directories to exclude from the check.', - valueHelp: 'path/to/dir', + valueHelp: 'path/to/dir (relative to the engine root)', defaultsTo: _engine != null ? [ - p.join(_engine!.flutterDir.path, 'build'), - p.join(_engine!.flutterDir.path, 'prebuilts'), - p.join(_engine!.flutterDir.path, 'third_party'), + 'build', + 'prebuilts', + 'third_party', ] : null, ); From 3afe63c1665f305b0d6fc46e8407ec94f1bf9584 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Dec 2023 17:15:52 -0800 Subject: [PATCH 08/26] ++ --- tools/header_guard_check/README.md | 12 +++ .../lib/header_guard_check.dart | 6 +- .../lib/src/header_file.dart | 88 ++++++++++++++++++- 3 files changed, 97 insertions(+), 9 deletions(-) diff --git a/tools/header_guard_check/README.md b/tools/header_guard_check/README.md index e0b7333458570..964ca65bc77f6 100644 --- a/tools/header_guard_check/README.md +++ b/tools/header_guard_check/README.md @@ -25,3 +25,15 @@ use this pattern, see [the Google C++ style guide](https://google.github.io/styl > [!IMPORTANT] > This is a prototype tool and is not yet integrated into the engine's CI, nor > provides a way to fix the header guards automatically. + +## Advanced usage + +### Restricting the files to check + +By default, the tool checks all header files in the engine. You can restrict the +files to check by passing a relative (to the `engine/src/flutter` root) paths to +include: + +```shell +dart ./tools/header_guard_check/bin/main.dart --include impeller +``` diff --git a/tools/header_guard_check/lib/header_guard_check.dart b/tools/header_guard_check/lib/header_guard_check.dart index 32c942ddb8b95..910430cf3eca0 100644 --- a/tools/header_guard_check/lib/header_guard_check.dart +++ b/tools/header_guard_check/lib/header_guard_check.dart @@ -91,11 +91,7 @@ final class HeaderGuardCheck { continue; } - // Determine what the expected guard should be. - // Find the relative path from the engine root to the file. - final String relativePath = p.relative(file.path, from: source.flutterDir.path); - final String underscoredRelativePath = relativePath.replaceAll(p.separator, '_'); - final String expectedGuard = 'FLUTTER_${p.withoutExtension(underscoredRelativePath).toUpperCase().replaceAll('.', '_')}_H_'; + final String expectedGuard = headerFile.expectedName(engineRoot: source.flutterDir.path); if (headerFile.guard!.ifndefValue != expectedGuard) { io.stderr.writeln(headerFile.guard!.ifndefSpan!.message('Expected #ifndef $expectedGuard')); badFiles.add(headerFile); diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index d5da4a9db2de7..e4036da026ea4 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -28,7 +28,8 @@ final class HeaderFile { } final String contents = file.readAsStringSync(); - final SourceFile sourceFile = SourceFile.fromString(contents, url: p.toUri(path)); + final SourceFile sourceFile = + SourceFile.fromString(contents, url: p.toUri(path)); return HeaderFile.from( path, guard: _parseGuard(sourceFile), @@ -67,9 +68,9 @@ final class HeaderFile { // Now iterate backwards to find the (last) #endif directive. for (int i = sourceFile.lines - 1; i > 0; i--) { final int start = sourceFile.getOffset(i); - final int end = i == sourceFile.lines - 1 ? - sourceFile.length : - sourceFile.getOffset(i + 1) - 1; + final int end = i == sourceFile.lines - 1 + ? sourceFile.length + : sourceFile.getOffset(i + 1) - 1; final String line = sourceFile.getText(start, end); // Check if the line is a header guard directive. @@ -118,6 +119,85 @@ final class HeaderFile { /// This is `null` if the file does not have a `#pragma once` directive. final SourceSpan? pragmaOnce; + /// Returns the expected header guard for this file, relative to [engineRoot]. + /// + /// For example, if the file is `foo/bar/baz.h`, this will return `FLUTTER_FOO_BAR_BAZ_H_`. + String expectedName({required String engineRoot}) { + final String relativePath = p.relative(path, from: engineRoot); + final String underscoredRelativePath = relativePath.replaceAll(p.separator, '_'); + return 'FLUTTER_${p.withoutExtension(underscoredRelativePath).toUpperCase().replaceAll('.', '_')}_H_'; + } + + /// Updates the file at [path] to have the expected header guard. + bool fix({required String engineRoot}) { + // Check if the file already has a valid header guard. + if (guard != null) { + final String expectedGuard = expectedName(engineRoot: engineRoot); + if (guard!.ifndefValue == expectedGuard && + guard!.defineValue == expectedGuard && + guard!.endifValue == expectedGuard) { + return false; + } + } + + // Get the contents of the file. + final String oldContents = io.File(path).readAsStringSync(); + + // If we're using pragma once, replace it with an ifndef/define, and + // append an endif and a newline at the end of the file. + if (pragmaOnce != null) { + final String expectedGuard = expectedName(engineRoot: engineRoot); + + // Append the endif and newline. + String newContents = '$oldContents\n#endif // $expectedGuard\n'; + + // Replace the span with the ifndef/define. + newContents = newContents.replaceRange( + pragmaOnce!.start.offset, + pragmaOnce!.end.offset, + '#ifndef $expectedGuard\n' + '#define $expectedGuard\n' + ); + + // Write the new contents to the file. + io.File(path).writeAsStringSync(newContents); + + return true; + } + + // If we're not using pragma once, replace the header guard with the + // expected header guard. + if (guard != null) { + final String expectedGuard = expectedName(engineRoot: engineRoot); + + // Replace endif: + String newContents = oldContents.replaceRange( + guard!.endifSpan!.start.offset, + guard!.endifSpan!.end.offset, + '#endif // $expectedGuard\n' + ); + + // Replace define: + newContents = newContents.replaceRange( + guard!.defineSpan!.start.offset, + guard!.defineSpan!.end.offset, + '#define $expectedGuard\n' + ); + + // Replace ifndef: + newContents = newContents.replaceRange( + guard!.ifndefSpan!.start.offset, + guard!.ifndefSpan!.end.offset, + '#ifndef $expectedGuard\n' + ); + + // Write the new contents to the file. + io.File(path).writeAsStringSync(newContents); + + return true; + } + } + @override bool operator ==(Object other) { return other is HeaderFile && From 59e98e6bd696f2736654ec78535d127e0d48dc65 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Dec 2023 17:17:17 -0800 Subject: [PATCH 09/26] ++ --- .../lib/header_guard_check.dart | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tools/header_guard_check/lib/header_guard_check.dart b/tools/header_guard_check/lib/header_guard_check.dart index 910430cf3eca0..13bdd10c99179 100644 --- a/tools/header_guard_check/lib/header_guard_check.dart +++ b/tools/header_guard_check/lib/header_guard_check.dart @@ -19,6 +19,7 @@ final class HeaderGuardCheck { required this.source, required this.exclude, this.include = const [], + this.fix = false, }); /// Parses the command line arguments and creates a new header guard checker. @@ -28,12 +29,16 @@ final class HeaderGuardCheck { source: Engine.fromSrcPath(argResults['root'] as String), include: argResults['include'] as List, exclude: argResults['exclude'] as List, + fix: argResults['fix'] as bool, ); } /// Engine source root. final Engine source; + /// Whether to automatically fix most header guards. + final bool fix; + /// Path directories to include in the check. final List include; @@ -114,6 +119,17 @@ final class HeaderGuardCheck { for (final HeaderFile headerFile in badFiles) { io.stdout.writeln(' ${headerFile.path}'); } + + // If we're fixing, fix the files. + if (fix) { + for (final HeaderFile headerFile in badFiles) { + headerFile.fix(engineRoot: source.flutterDir.path); + } + + io.stdout.writeln('Fixed ${badFiles.length} files.'); + return 0; + } + return 1; } @@ -124,6 +140,10 @@ final class HeaderGuardCheck { final Engine? _engine = Engine.tryFindWithin(p.dirname(p.fromUri(io.Platform.script))); final ArgParser _parser = ArgParser() + ..addFlag( + 'fix', + help: 'Automatically fixes most header guards.', + ) ..addOption( 'root', abbr: 'r', From acc939ac8781079c47e337f0f0aa71e30e5c3489 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Dec 2023 17:17:57 -0800 Subject: [PATCH 10/26] ++ --- tools/header_guard_check/lib/src/header_file.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index e4036da026ea4..4dc3dbc4a3cfd 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -196,6 +196,8 @@ final class HeaderFile { return true; } + + return false; } @override From 8d3b0f9e941401fe5b400afc0fe300f9ae3958ca Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Dec 2023 17:20:22 -0800 Subject: [PATCH 11/26] ++ --- tools/pub_get_offline.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/pub_get_offline.py b/tools/pub_get_offline.py index 931db7319b8f9..f5e41ba3245e1 100644 --- a/tools/pub_get_offline.py +++ b/tools/pub_get_offline.py @@ -40,6 +40,7 @@ os.path.join(ENGINE_DIR, 'tools', 'const_finder'), os.path.join(ENGINE_DIR, 'tools', 'gen_web_locale_keymap'), os.path.join(ENGINE_DIR, 'tools', 'githooks'), + os.path.join(ENGINE_DIR, 'tools', 'header_guard_check'), os.path.join(ENGINE_DIR, 'tools', 'licenses'), os.path.join(ENGINE_DIR, 'tools', 'path_ops', 'dart'), os.path.join(ENGINE_DIR, 'tools', 'pkg', 'engine_build_configs'), From 1c8fa0ebbe3d97bf2ebf9fde2c87a7a2318ca720 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Dec 2023 17:24:11 -0800 Subject: [PATCH 12/26] ++ --- .../header_guard_check/lib/header_guard_check.dart | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tools/header_guard_check/lib/header_guard_check.dart b/tools/header_guard_check/lib/header_guard_check.dart index 13bdd10c99179..34d27262d7126 100644 --- a/tools/header_guard_check/lib/header_guard_check.dart +++ b/tools/header_guard_check/lib/header_guard_check.dart @@ -57,7 +57,7 @@ final class HeaderGuardCheck { bool included = include.isEmpty; for (final String includePath in include) { final String relativePath = p.relative(includePath, from: source.flutterDir.path); - if (p.isWithin(relativePath, entity.path)) { + if (p.isWithin(relativePath, entity.path) || p.equals(relativePath, entity.path)) { included = true; break; } @@ -70,7 +70,7 @@ final class HeaderGuardCheck { bool excluded = false; for (final String excludePath in exclude) { final String relativePath = p.relative(excludePath, from: source.flutterDir.path); - if (p.isWithin(relativePath, entity.path)) { + if (p.isWithin(relativePath, entity.path) || p.equals(relativePath, entity.path)) { excluded = true; break; } @@ -154,17 +154,18 @@ final ArgParser _parser = ArgParser() ..addMultiOption( 'include', abbr: 'i', - help: 'Path directories to include in the check.', - valueHelp: 'path/to/dir (relative to the engine root)', + help: 'Paths to include in the check.', + valueHelp: 'path/to/dir/or/file (relative to the engine root)', defaultsTo: [], ) ..addMultiOption( 'exclude', abbr: 'e', - help: 'Path directories to exclude from the check.', - valueHelp: 'path/to/dir (relative to the engine root)', + help: 'Paths to exclude from the check.', + valueHelp: 'path/to/dir/or/file (relative to the engine root)', defaultsTo: _engine != null ? [ 'build', + 'impeller/compiler/code_gen_template.h', 'prebuilts', 'third_party', ] : null, From 7d1a6218161bb20dd52c7c0bd781ecd84497fe36 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Dec 2023 17:30:20 -0800 Subject: [PATCH 13/26] Change WS. --- tools/header_guard_check/lib/src/header_file.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index 4dc3dbc4a3cfd..026d8bb324a5e 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -156,7 +156,7 @@ final class HeaderFile { pragmaOnce!.start.offset, pragmaOnce!.end.offset, '#ifndef $expectedGuard\n' - '#define $expectedGuard\n' + '#define $expectedGuard' ); // Write the new contents to the file. @@ -181,7 +181,7 @@ final class HeaderFile { newContents = newContents.replaceRange( guard!.defineSpan!.start.offset, guard!.defineSpan!.end.offset, - '#define $expectedGuard\n' + '#define $expectedGuard' ); // Replace ifndef: From 68ce106c59d3759383d55168d05fa541b40362d6 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 12 Dec 2023 17:59:26 -0800 Subject: [PATCH 14/26] ++ --- tools/header_guard_check/lib/src/header_file.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index 026d8bb324a5e..b51779865e5c9 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -147,7 +147,7 @@ final class HeaderFile { // append an endif and a newline at the end of the file. if (pragmaOnce != null) { final String expectedGuard = expectedName(engineRoot: engineRoot); - + // Append the endif and newline. String newContents = '$oldContents\n#endif // $expectedGuard\n'; @@ -169,7 +169,7 @@ final class HeaderFile { // expected header guard. if (guard != null) { final String expectedGuard = expectedName(engineRoot: engineRoot); - + // Replace endif: String newContents = oldContents.replaceRange( guard!.endifSpan!.start.offset, From 5212adee2514e31365e81f03278640e43506e576 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 13 Dec 2023 16:54:07 -0800 Subject: [PATCH 15/26] Tweak WS. --- tools/header_guard_check/lib/src/header_file.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index b51779865e5c9..df20212cd3eaa 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -174,7 +174,7 @@ final class HeaderFile { String newContents = oldContents.replaceRange( guard!.endifSpan!.start.offset, guard!.endifSpan!.end.offset, - '#endif // $expectedGuard\n' + '#endif // $expectedGuard' ); // Replace define: @@ -188,7 +188,7 @@ final class HeaderFile { newContents = newContents.replaceRange( guard!.ifndefSpan!.start.offset, guard!.ifndefSpan!.end.offset, - '#ifndef $expectedGuard\n' + '#ifndef $expectedGuard' ); // Write the new contents to the file. From f9bfce155c1af691990231718e3300cc71a4f84f Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Dec 2023 13:56:49 -0800 Subject: [PATCH 16/26] Tweak expected name. --- .../lib/src/header_file.dart | 9 +++++---- .../test/header_file_test.dart | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index df20212cd3eaa..a2b0a90e0af2f 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -28,8 +28,7 @@ final class HeaderFile { } final String contents = file.readAsStringSync(); - final SourceFile sourceFile = - SourceFile.fromString(contents, url: p.toUri(path)); + final SourceFile sourceFile = SourceFile.fromString(contents, url: p.toUri(path)); return HeaderFile.from( path, guard: _parseGuard(sourceFile), @@ -119,13 +118,15 @@ final class HeaderFile { /// This is `null` if the file does not have a `#pragma once` directive. final SourceSpan? pragmaOnce; + static final RegExp _nonAlphaNumeric = RegExp(r'[^a-zA-Z0-9]'); + /// Returns the expected header guard for this file, relative to [engineRoot]. /// /// For example, if the file is `foo/bar/baz.h`, this will return `FLUTTER_FOO_BAR_BAZ_H_`. String expectedName({required String engineRoot}) { final String relativePath = p.relative(path, from: engineRoot); - final String underscoredRelativePath = relativePath.replaceAll(p.separator, '_'); - return 'FLUTTER_${p.withoutExtension(underscoredRelativePath).toUpperCase().replaceAll('.', '_')}_H_'; + final String underscoredRelativePath = p.withoutExtension(relativePath).replaceAll(_nonAlphaNumeric, '_'); + return 'FLUTTER_${underscoredRelativePath.toUpperCase()}_H_'; } /// Updates the file at [path] to have the expected header guard. diff --git a/tools/header_guard_check/test/header_file_test.dart b/tools/header_guard_check/test/header_file_test.dart index a8abd41c02475..cda4748952fe2 100644 --- a/tools/header_guard_check/test/header_file_test.dart +++ b/tools/header_guard_check/test/header_file_test.dart @@ -142,6 +142,25 @@ Future main(List args) async { }); group('HeaderFile', () { + test('produces a valid header guard name from various file names', () { + // All of these should produce the name `FOO_BAR_BAZ_H_`. + const List inputs = [ + 'foo_bar_baz.h', + 'foo-bar-baz.h', + 'foo_bar-baz.h', + 'foo-bar_baz.h', + 'foo+bar+baz.h', + ]; + for (final String input in inputs) { + final HeaderFile headerFile = HeaderFile.from( + input, + guard: null, + pragmaOnce: null, + ); + expect(headerFile.expectedName(engineRoot: ''), endsWith('FOO_BAR_BAZ_H_')); + } + }); + test('parses a header file with a valid guard', () { final String input = [ '#ifndef FOO_H_', From 1b6ea20d40e8217ffc59d31b241f645ca61ecdf7 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Dec 2023 13:58:10 -0800 Subject: [PATCH 17/26] ++ --- tools/header_guard_check/test/header_file_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/header_guard_check/test/header_file_test.dart b/tools/header_guard_check/test/header_file_test.dart index cda4748952fe2..ff7999e08c89f 100644 --- a/tools/header_guard_check/test/header_file_test.dart +++ b/tools/header_guard_check/test/header_file_test.dart @@ -153,7 +153,7 @@ Future main(List args) async { ]; for (final String input in inputs) { final HeaderFile headerFile = HeaderFile.from( - input, + input, guard: null, pragmaOnce: null, ); From de041721a717ac84616e427c50fe58c7da720da5 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Dec 2023 14:00:55 -0800 Subject: [PATCH 18/26] ++ --- tools/header_guard_check/README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/header_guard_check/README.md b/tools/header_guard_check/README.md index 964ca65bc77f6..a8354b7ee5b40 100644 --- a/tools/header_guard_check/README.md +++ b/tools/header_guard_check/README.md @@ -23,8 +23,7 @@ message and exit with a non-zero exit code. For more information about why we use this pattern, see [the Google C++ style guide](https://google.github.io/styleguide/cppguide.html#The__define_Guard). > [!IMPORTANT] -> This is a prototype tool and is not yet integrated into the engine's CI, nor -> provides a way to fix the header guards automatically. +> This is a prototype tool and is not yet integrated into the engine's CI. ## Advanced usage From dd2b5b713baf89dc6e302f0c98a0628744225d32 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Dec 2023 14:04:38 -0800 Subject: [PATCH 19/26] ++ --- tools/header_guard_check/README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tools/header_guard_check/README.md b/tools/header_guard_check/README.md index a8354b7ee5b40..20ec093564f0e 100644 --- a/tools/header_guard_check/README.md +++ b/tools/header_guard_check/README.md @@ -25,6 +25,14 @@ use this pattern, see [the Google C++ style guide](https://google.github.io/styl > [!IMPORTANT] > This is a prototype tool and is not yet integrated into the engine's CI. +## Automatic fixes + +The tool can automatically fix header files that do not follow the pattern: + +```shell +dart ./tools/header_guard_check/bin/main.dart --fix +``` + ## Advanced usage ### Restricting the files to check From 713416dec8022949f053c49cfeaad7a8b6996f5e Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Dec 2023 14:51:06 -0800 Subject: [PATCH 20/26] ++ --- tools/header_guard_check/pubspec.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/header_guard_check/pubspec.yaml b/tools/header_guard_check/pubspec.yaml index 2796069a76580..52e00456a9548 100644 --- a/tools/header_guard_check/pubspec.yaml +++ b/tools/header_guard_check/pubspec.yaml @@ -48,6 +48,8 @@ dependency_overrides: path: ../pkg/git_repo_tools litetest: path: ../../testing/litetest + meta: + path: ../../../third_party/dart/pkg/meta path: path: ../../../third_party/dart/third_party/pkg/path platform: From 03b909905a3c6811298085d6249ca368d9ce1bd5 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 25 Jan 2024 17:30:46 -0800 Subject: [PATCH 21/26] Tweak rules. --- .../lib/src/header_file.dart | 25 ++++-- .../test/header_file_test.dart | 76 +++++++++++++++++++ 2 files changed, 93 insertions(+), 8 deletions(-) diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index a2b0a90e0af2f..b3dfd44ee4fd9 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -131,9 +131,10 @@ final class HeaderFile { /// Updates the file at [path] to have the expected header guard. bool fix({required String engineRoot}) { + final String expectedGuard = expectedName(engineRoot: engineRoot); + // Check if the file already has a valid header guard. if (guard != null) { - final String expectedGuard = expectedName(engineRoot: engineRoot); if (guard!.ifndefValue == expectedGuard && guard!.defineValue == expectedGuard && guard!.endifValue == expectedGuard) { @@ -147,8 +148,6 @@ final class HeaderFile { // If we're using pragma once, replace it with an ifndef/define, and // append an endif and a newline at the end of the file. if (pragmaOnce != null) { - final String expectedGuard = expectedName(engineRoot: engineRoot); - // Append the endif and newline. String newContents = '$oldContents\n#endif // $expectedGuard\n'; @@ -162,14 +161,12 @@ final class HeaderFile { // Write the new contents to the file. io.File(path).writeAsStringSync(newContents); - return true; } // If we're not using pragma once, replace the header guard with the // expected header guard. if (guard != null) { - final String expectedGuard = expectedName(engineRoot: engineRoot); // Replace endif: String newContents = oldContents.replaceRange( @@ -193,12 +190,24 @@ final class HeaderFile { ); // Write the new contents to the file. - io.File(path).writeAsStringSync(newContents); - + io.File(path).writeAsStringSync('$newContents\n'); return true; } - return false; + // If we're missing a guard entirely, add one. The rules are: + // 1. Add a newline, #endif at the end of the file. + // 2. Add a newline, #ifndef, #define after the first non-comment line. + String newContents = oldContents; + newContents += '\n#endif // $expectedGuard\n'; + newContents = newContents.replaceFirst( + RegExp(r'^(?!//)', multiLine: true), + '\n#ifndef $expectedGuard\n' + '#define $expectedGuard\n' + ); + + // Write the new contents to the file. + io.File(path).writeAsStringSync(newContents); + return true; } @override diff --git a/tools/header_guard_check/test/header_file_test.dart b/tools/header_guard_check/test/header_file_test.dart index ff7999e08c89f..d6c5660779203 100644 --- a/tools/header_guard_check/test/header_file_test.dart +++ b/tools/header_guard_check/test/header_file_test.dart @@ -230,6 +230,82 @@ Future main(List args) async { expect(headerFile.pragmaOnce, isNotNull); }); }); + + test('fixes a file that uses #pragma once', () { + final String input = [ + '#pragma once', + '', + '// ...', + ].join('\n'); + withTestFile('foo.h', input, (io.File file) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + expect(headerFile.fix(engineRoot: p.dirname(file.path)), isTrue); + expect(file.readAsStringSync(), [ + '#ifndef FLUTTER_FOO_H_', + '#define FLUTTER_FOO_H_', + '', + '// ...', + '#endif // FLUTTER_FOO_H_', + '', + ].join('\n')); + }); + }); + + test('fixes a file with an incorrect header guard', () { + final String input = [ + '#ifndef FOO_H_', + '#define FOO_H_', + '', + '#endif // FOO_H_', + ].join('\n'); + withTestFile('foo.h', input, (io.File file) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + expect(headerFile.fix(engineRoot: p.dirname(file.path)), isTrue); + expect(file.readAsStringSync(), [ + '#ifndef FLUTTER_FOO_H_', + '#define FLUTTER_FOO_H_', + '', + '#endif // FLUTTER_FOO_H_', + '', + ].join('\n')); + }); + }); + + test('fixes a file with no header guard', () { + final String input = [ + '// 1.', + '// 2.', + '// 3.', + '', + "#import 'flutter/shell/platform/darwin/Flutter.h'", + '', + '@protocl Flutter', + '', + '@end', + '', + ].join('\n'); + withTestFile('foo.h', input, (io.File file) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + expect(headerFile.fix(engineRoot: p.dirname(file.path)), isTrue); + expect(file.readAsStringSync(), [ + '// 1.', + '// 2.', + '// 3.', + '', + '#ifndef FLUTTER_FOO_H_', + '#define FLUTTER_FOO_H_', + '', + "#import 'flutter/shell/platform/darwin/Flutter.h'", + '', + '@protocl Flutter', + '', + '@end', + '', + '#endif // FLUTTER_FOO_H_', + '', + ].join('\n')); + }); + }); }); return 0; From 6593be63f2f21dd55fb4ebc4addb0e018c564e61 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 25 Jan 2024 17:51:54 -0800 Subject: [PATCH 22/26] ++ --- .../lib/src/header_file.dart | 4 ++++ .../test/header_file_test.dart | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index b3dfd44ee4fd9..82cb6bb1abca5 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -54,6 +54,10 @@ final class HeaderFile { if (line.startsWith('#ifndef')) { ifndefSpan = sourceFile.span(start, end); } else if (line.startsWith('#define')) { + // If we find a define preceding an ifndef, it is not a header guard. + if (ifndefSpan == null) { + continue; + } defineSpan = sourceFile.span(start, end); break; } diff --git a/tools/header_guard_check/test/header_file_test.dart b/tools/header_guard_check/test/header_file_test.dart index d6c5660779203..11bb11ede0c70 100644 --- a/tools/header_guard_check/test/header_file_test.dart +++ b/tools/header_guard_check/test/header_file_test.dart @@ -306,6 +306,26 @@ Future main(List args) async { ].join('\n')); }); }); + + test('does not touch a file with an existing guard and another #define', () { + final String input = [ + '// 1.', + '// 2.', + '// 3.', + '', + '#define FML_USED_ON_EMBEDDER', + '', + '#ifndef FLUTTER_FOO_H_', + '#define FLUTTER_FOO_H_', + '', + '#endif // FLUTTER_FOO_H_', + '', + ].join('\n'); + withTestFile('foo.h', input, (io.File file) { + final HeaderFile headerFile = HeaderFile.parse(file.path); + expect(headerFile.fix(engineRoot: p.dirname(file.path)), isFalse); + }); + }); }); return 0; From 26fa5288de3b8112a9262ce0dccb2f1519399eca Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 26 Jan 2024 11:37:50 -0800 Subject: [PATCH 23/26] Address feedback. --- testing/run_tests.py | 1 + .../lib/header_guard_check.dart | 131 +++++++++--------- .../lib/src/header_file.dart | 6 +- .../test/header_file_test.dart | 2 +- 4 files changed, 73 insertions(+), 67 deletions(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 2517c091123ce..b829dfd0dc447 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -1004,6 +1004,7 @@ def build_dart_host_test_list(build_dir): (os.path.join('flutter', 'tools', 'pkg', 'engine_build_configs'), []), (os.path.join('flutter', 'tools', 'pkg', 'engine_repo_tools'), []), (os.path.join('flutter', 'tools', 'pkg', 'git_repo_tools'), []), + (os.path.join('flutter', 'tools', 'pkg', 'header_guard_check'), []), ] if not is_asan(build_dir): dart_host_tests += [ diff --git a/tools/header_guard_check/lib/header_guard_check.dart b/tools/header_guard_check/lib/header_guard_check.dart index 34d27262d7126..396d586890537 100644 --- a/tools/header_guard_check/lib/header_guard_check.dart +++ b/tools/header_guard_check/lib/header_guard_check.dart @@ -47,93 +47,96 @@ final class HeaderGuardCheck { /// Runs the header guard check. Future run() async { - final List files = []; + final List badFiles = _checkFiles(_findIncludedHeaderFiles()).toList(); - // Recursive search for header files. - final io.Directory dir = source.flutterDir; - await for (final io.FileSystemEntity entity in dir.list(recursive: true)) { - if (entity is io.File && entity.path.endsWith('.h')) { - // Check that the file is included. - bool included = include.isEmpty; - for (final String includePath in include) { - final String relativePath = p.relative(includePath, from: source.flutterDir.path); - if (p.isWithin(relativePath, entity.path) || p.equals(relativePath, entity.path)) { - included = true; - break; - } - } - if (!included) { - continue; - } + if (badFiles.isNotEmpty) { + io.stdout.writeln('The following ${badFiles.length} files have invalid header guards:'); + for (final HeaderFile headerFile in badFiles) { + io.stdout.writeln(' ${headerFile.path}'); + } - // Check that the file is not excluded. - bool excluded = false; - for (final String excludePath in exclude) { - final String relativePath = p.relative(excludePath, from: source.flutterDir.path); - if (p.isWithin(relativePath, entity.path) || p.equals(relativePath, entity.path)) { - excluded = true; - break; - } - } - if (!excluded) { - files.add(entity); + // If we're fixing, fix the files. + if (fix) { + for (final HeaderFile headerFile in badFiles) { + headerFile.fix(engineRoot: source.flutterDir.path); } + + io.stdout.writeln('Fixed ${badFiles.length} files.'); + return 0; + } + + return 1; + } + + return 0; + } + + Iterable _findIncludedHeaderFiles() sync* { + final io.Directory dir = source.flutterDir; + for (final io.FileSystemEntity entity in dir.listSync(recursive: true)) { + if (entity is! io.File) { + continue; + } + + if (!entity.path.endsWith('.h')) { + continue; + } + + if (!_isIncluded(entity.path) || _isExcluded(entity.path)) { + continue; + } + + yield entity; + } + } + + bool _isIncluded(String path) { + for (final String includePath in include) { + final String relativePath = p.relative(includePath, from: source.flutterDir.path); + if (p.isWithin(relativePath, path) || p.equals(relativePath, path)) { + return true; } } + return include.isEmpty; + } - // Check each file. - final List badFiles = []; - for (final io.File file in files) { - final HeaderFile headerFile = HeaderFile.parse(file.path); + bool _isExcluded(String path) { + for (final String excludePath in exclude) { + final String relativePath = p.relative(excludePath, from: source.flutterDir.path); + if (p.isWithin(relativePath, path) || p.equals(relativePath, path)) { + return true; + } + } + return false; + } + + Iterable _checkFiles(Iterable headers) sync* { + for (final io.File header in headers) { + final HeaderFile headerFile = HeaderFile.parse(header.path); if (headerFile.pragmaOnce != null) { io.stderr.writeln(headerFile.pragmaOnce!.message('Unexpected #pragma once')); - badFiles.add(headerFile); - continue; + yield headerFile; } + if (headerFile.guard == null) { io.stderr.writeln('Missing header guard in ${headerFile.path}'); - badFiles.add(headerFile); - continue; + yield headerFile; } - final String expectedGuard = headerFile.expectedName(engineRoot: source.flutterDir.path); + final String expectedGuard = headerFile.computeExpectedName(engineRoot: source.flutterDir.path); if (headerFile.guard!.ifndefValue != expectedGuard) { io.stderr.writeln(headerFile.guard!.ifndefSpan!.message('Expected #ifndef $expectedGuard')); - badFiles.add(headerFile); - continue; + yield headerFile; } if (headerFile.guard!.defineValue != expectedGuard) { io.stderr.writeln(headerFile.guard!.defineSpan!.message('Expected #define $expectedGuard')); - badFiles.add(headerFile); - continue; + yield headerFile; } if (headerFile.guard!.endifValue != expectedGuard) { io.stderr.writeln(headerFile.guard!.endifSpan!.message('Expected #endif // $expectedGuard')); - badFiles.add(headerFile); - continue; - } - } - - if (badFiles.isNotEmpty) { - io.stdout.writeln('The following ${badFiles.length} files have invalid header guards:'); - for (final HeaderFile headerFile in badFiles) { - io.stdout.writeln(' ${headerFile.path}'); + yield headerFile; } - - // If we're fixing, fix the files. - if (fix) { - for (final HeaderFile headerFile in badFiles) { - headerFile.fix(engineRoot: source.flutterDir.path); - } - - io.stdout.writeln('Fixed ${badFiles.length} files.'); - return 0; - } - - return 1; } - - return 0; } } diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index 82cb6bb1abca5..16cd45302ded4 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -127,15 +127,17 @@ final class HeaderFile { /// Returns the expected header guard for this file, relative to [engineRoot]. /// /// For example, if the file is `foo/bar/baz.h`, this will return `FLUTTER_FOO_BAR_BAZ_H_`. - String expectedName({required String engineRoot}) { + String computeExpectedName({required String engineRoot}) { final String relativePath = p.relative(path, from: engineRoot); final String underscoredRelativePath = p.withoutExtension(relativePath).replaceAll(_nonAlphaNumeric, '_'); return 'FLUTTER_${underscoredRelativePath.toUpperCase()}_H_'; } /// Updates the file at [path] to have the expected header guard. + /// + /// Returns `true` if the file was modified, `false` otherwise. bool fix({required String engineRoot}) { - final String expectedGuard = expectedName(engineRoot: engineRoot); + final String expectedGuard = computeExpectedName(engineRoot: engineRoot); // Check if the file already has a valid header guard. if (guard != null) { diff --git a/tools/header_guard_check/test/header_file_test.dart b/tools/header_guard_check/test/header_file_test.dart index 11bb11ede0c70..4342ba3ed40b5 100644 --- a/tools/header_guard_check/test/header_file_test.dart +++ b/tools/header_guard_check/test/header_file_test.dart @@ -157,7 +157,7 @@ Future main(List args) async { guard: null, pragmaOnce: null, ); - expect(headerFile.expectedName(engineRoot: ''), endsWith('FOO_BAR_BAZ_H_')); + expect(headerFile.computeExpectedName(engineRoot: ''), endsWith('FOO_BAR_BAZ_H_')); } }); From 1d1627a3bbf29a7a598929ff9ec4db993e9b1669 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 26 Jan 2024 11:38:12 -0800 Subject: [PATCH 24/26] ++ --- tools/header_guard_check/lib/header_guard_check.dart | 2 +- tools/header_guard_check/lib/src/header_file.dart | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/header_guard_check/lib/header_guard_check.dart b/tools/header_guard_check/lib/header_guard_check.dart index 396d586890537..f7880c7fe562d 100644 --- a/tools/header_guard_check/lib/header_guard_check.dart +++ b/tools/header_guard_check/lib/header_guard_check.dart @@ -70,7 +70,7 @@ final class HeaderGuardCheck { return 0; } - + Iterable _findIncludedHeaderFiles() sync* { final io.Directory dir = source.flutterDir; for (final io.FileSystemEntity entity in dir.listSync(recursive: true)) { diff --git a/tools/header_guard_check/lib/src/header_file.dart b/tools/header_guard_check/lib/src/header_file.dart index 16cd45302ded4..9ab0b6318b015 100644 --- a/tools/header_guard_check/lib/src/header_file.dart +++ b/tools/header_guard_check/lib/src/header_file.dart @@ -134,7 +134,7 @@ final class HeaderFile { } /// Updates the file at [path] to have the expected header guard. - /// + /// /// Returns `true` if the file was modified, `false` otherwise. bool fix({required String engineRoot}) { final String expectedGuard = computeExpectedName(engineRoot: engineRoot); From f91b3fda1ff43958f18f45f718132c7f06e7846f Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 26 Jan 2024 12:01:03 -0800 Subject: [PATCH 25/26] ++ --- testing/run_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index b829dfd0dc447..bc5fa72269889 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -1001,10 +1001,10 @@ def build_dart_host_test_list(build_dir): ], ), (os.path.join('flutter', 'tools', 'githooks'), []), + (os.path.join('flutter', 'tools', 'header_guard_check'), []), (os.path.join('flutter', 'tools', 'pkg', 'engine_build_configs'), []), (os.path.join('flutter', 'tools', 'pkg', 'engine_repo_tools'), []), (os.path.join('flutter', 'tools', 'pkg', 'git_repo_tools'), []), - (os.path.join('flutter', 'tools', 'pkg', 'header_guard_check'), []), ] if not is_asan(build_dir): dart_host_tests += [ From 7107ef1beff4299237b9a65b4afce6cc3069f6a7 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 26 Jan 2024 12:43:49 -0800 Subject: [PATCH 26/26] ++ --- tools/header_guard_check/pubspec.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/header_guard_check/pubspec.yaml b/tools/header_guard_check/pubspec.yaml index 52e00456a9548..d775e403853e2 100644 --- a/tools/header_guard_check/pubspec.yaml +++ b/tools/header_guard_check/pubspec.yaml @@ -34,6 +34,8 @@ dev_dependencies: dependency_overrides: args: path: ../../../third_party/dart/third_party/pkg/args + async: + path: ../../../third_party/dart/third_party/pkg/async async_helper: path: ../../../third_party/dart/pkg/async_helper collection: