Skip to content

Commit d33b2dd

Browse files
authored
Introduce a prototype of a "header guard enforcement" tool (flutter#48903)
Closes flutter#133415. --- This is a prototype I threw together in about 1-2 hours. It enforces and automatically fixes header guards that don't match the [the Google C++ style guide](https://google.github.io/styleguide/cppguide.html#The__define_Guard). For example, here is (trimmed) output at HEAD: ```txt line 5, column 1 of impeller/aiks/picture.h: Unexpected #pragma once ╷ 5 │ #pragma once │ ^^^^^^^^^^^^ ╵ line 5, column 1 of flow/stopwatch.h: Expected #ifndef FLUTTER_FLOW_STOPWATCH_H_ ╷ 5 │ #ifndef FLUTTER_FLOW_INSTRUMENTATION_H_ │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ╵ The following 731 files have invalid header guards: ```
1 parent daf139b commit d33b2dd

File tree

8 files changed

+966
-0
lines changed

8 files changed

+966
-0
lines changed

testing/run_tests.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,6 +1001,7 @@ def build_dart_host_test_list(build_dir):
10011001
],
10021002
),
10031003
(os.path.join('flutter', 'tools', 'githooks'), []),
1004+
(os.path.join('flutter', 'tools', 'header_guard_check'), []),
10041005
(os.path.join('flutter', 'tools', 'pkg', 'engine_build_configs'), []),
10051006
(os.path.join('flutter', 'tools', 'pkg', 'engine_repo_tools'), []),
10061007
(os.path.join('flutter', 'tools', 'pkg', 'git_repo_tools'), []),

tools/header_guard_check/README.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
# header_guard_check
2+
3+
A tool to check that C++ header guards are used consistently in the engine.
4+
5+
```shell
6+
# Assuming you are in the `flutter` root of the engine repo.
7+
dart ./tools/header_guard_check/bin/main.dart
8+
```
9+
10+
The tool checks _all_ header files for the following pattern:
11+
12+
```h
13+
// path/to/file.h
14+
15+
#ifndef PATH_TO_FILE_H_
16+
#define PATH_TO_FILE_H_
17+
...
18+
#endif // PATH_TO_FILE_H_
19+
```
20+
21+
If the header file does not follow this pattern, the tool will print an error
22+
message and exit with a non-zero exit code. For more information about why we
23+
use this pattern, see [the Google C++ style guide](https://google.github.io/styleguide/cppguide.html#The__define_Guard).
24+
25+
> [!IMPORTANT]
26+
> This is a prototype tool and is not yet integrated into the engine's CI.
27+
28+
## Automatic fixes
29+
30+
The tool can automatically fix header files that do not follow the pattern:
31+
32+
```shell
33+
dart ./tools/header_guard_check/bin/main.dart --fix
34+
```
35+
36+
## Advanced usage
37+
38+
### Restricting the files to check
39+
40+
By default, the tool checks all header files in the engine. You can restrict the
41+
files to check by passing a relative (to the `engine/src/flutter` root) paths to
42+
include:
43+
44+
```shell
45+
dart ./tools/header_guard_check/bin/main.dart --include impeller
46+
```
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'dart:io' as io;
6+
7+
import 'package:header_guard_check/header_guard_check.dart';
8+
9+
Future<int> main(List<String> arguments) async {
10+
final int result = await HeaderGuardCheck.fromCommandLine(arguments).run();
11+
if (result != 0) {
12+
io.exit(result);
13+
}
14+
return result;
15+
}
Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'dart:io' as io;
6+
7+
import 'package:args/args.dart';
8+
import 'package:engine_repo_tools/engine_repo_tools.dart';
9+
import 'package:meta/meta.dart';
10+
import 'package:path/path.dart' as p;
11+
12+
import 'src/header_file.dart';
13+
14+
/// Checks C++ header files for header guards.
15+
@immutable
16+
final class HeaderGuardCheck {
17+
/// Creates a new header guard checker.
18+
const HeaderGuardCheck({
19+
required this.source,
20+
required this.exclude,
21+
this.include = const <String>[],
22+
this.fix = false,
23+
});
24+
25+
/// Parses the command line arguments and creates a new header guard checker.
26+
factory HeaderGuardCheck.fromCommandLine(List<String> arguments) {
27+
final ArgResults argResults = _parser.parse(arguments);
28+
return HeaderGuardCheck(
29+
source: Engine.fromSrcPath(argResults['root'] as String),
30+
include: argResults['include'] as List<String>,
31+
exclude: argResults['exclude'] as List<String>,
32+
fix: argResults['fix'] as bool,
33+
);
34+
}
35+
36+
/// Engine source root.
37+
final Engine source;
38+
39+
/// Whether to automatically fix most header guards.
40+
final bool fix;
41+
42+
/// Path directories to include in the check.
43+
final List<String> include;
44+
45+
/// Path directories to exclude from the check.
46+
final List<String> exclude;
47+
48+
/// Runs the header guard check.
49+
Future<int> run() async {
50+
final List<HeaderFile> badFiles = _checkFiles(_findIncludedHeaderFiles()).toList();
51+
52+
if (badFiles.isNotEmpty) {
53+
io.stdout.writeln('The following ${badFiles.length} files have invalid header guards:');
54+
for (final HeaderFile headerFile in badFiles) {
55+
io.stdout.writeln(' ${headerFile.path}');
56+
}
57+
58+
// If we're fixing, fix the files.
59+
if (fix) {
60+
for (final HeaderFile headerFile in badFiles) {
61+
headerFile.fix(engineRoot: source.flutterDir.path);
62+
}
63+
64+
io.stdout.writeln('Fixed ${badFiles.length} files.');
65+
return 0;
66+
}
67+
68+
return 1;
69+
}
70+
71+
return 0;
72+
}
73+
74+
Iterable<io.File> _findIncludedHeaderFiles() sync* {
75+
final io.Directory dir = source.flutterDir;
76+
for (final io.FileSystemEntity entity in dir.listSync(recursive: true)) {
77+
if (entity is! io.File) {
78+
continue;
79+
}
80+
81+
if (!entity.path.endsWith('.h')) {
82+
continue;
83+
}
84+
85+
if (!_isIncluded(entity.path) || _isExcluded(entity.path)) {
86+
continue;
87+
}
88+
89+
yield entity;
90+
}
91+
}
92+
93+
bool _isIncluded(String path) {
94+
for (final String includePath in include) {
95+
final String relativePath = p.relative(includePath, from: source.flutterDir.path);
96+
if (p.isWithin(relativePath, path) || p.equals(relativePath, path)) {
97+
return true;
98+
}
99+
}
100+
return include.isEmpty;
101+
}
102+
103+
bool _isExcluded(String path) {
104+
for (final String excludePath in exclude) {
105+
final String relativePath = p.relative(excludePath, from: source.flutterDir.path);
106+
if (p.isWithin(relativePath, path) || p.equals(relativePath, path)) {
107+
return true;
108+
}
109+
}
110+
return false;
111+
}
112+
113+
Iterable<HeaderFile> _checkFiles(Iterable<io.File> headers) sync* {
114+
for (final io.File header in headers) {
115+
final HeaderFile headerFile = HeaderFile.parse(header.path);
116+
if (headerFile.pragmaOnce != null) {
117+
io.stderr.writeln(headerFile.pragmaOnce!.message('Unexpected #pragma once'));
118+
yield headerFile;
119+
}
120+
121+
if (headerFile.guard == null) {
122+
io.stderr.writeln('Missing header guard in ${headerFile.path}');
123+
yield headerFile;
124+
}
125+
126+
final String expectedGuard = headerFile.computeExpectedName(engineRoot: source.flutterDir.path);
127+
if (headerFile.guard!.ifndefValue != expectedGuard) {
128+
io.stderr.writeln(headerFile.guard!.ifndefSpan!.message('Expected #ifndef $expectedGuard'));
129+
yield headerFile;
130+
}
131+
if (headerFile.guard!.defineValue != expectedGuard) {
132+
io.stderr.writeln(headerFile.guard!.defineSpan!.message('Expected #define $expectedGuard'));
133+
yield headerFile;
134+
}
135+
if (headerFile.guard!.endifValue != expectedGuard) {
136+
io.stderr.writeln(headerFile.guard!.endifSpan!.message('Expected #endif // $expectedGuard'));
137+
yield headerFile;
138+
}
139+
}
140+
}
141+
}
142+
143+
final Engine? _engine = Engine.tryFindWithin(p.dirname(p.fromUri(io.Platform.script)));
144+
145+
final ArgParser _parser = ArgParser()
146+
..addFlag(
147+
'fix',
148+
help: 'Automatically fixes most header guards.',
149+
)
150+
..addOption(
151+
'root',
152+
abbr: 'r',
153+
help: 'Path to the engine source root.',
154+
valueHelp: 'path/to/engine/src',
155+
defaultsTo: _engine?.srcDir.path,
156+
)
157+
..addMultiOption(
158+
'include',
159+
abbr: 'i',
160+
help: 'Paths to include in the check.',
161+
valueHelp: 'path/to/dir/or/file (relative to the engine root)',
162+
defaultsTo: <String>[],
163+
)
164+
..addMultiOption(
165+
'exclude',
166+
abbr: 'e',
167+
help: 'Paths to exclude from the check.',
168+
valueHelp: 'path/to/dir/or/file (relative to the engine root)',
169+
defaultsTo: _engine != null ? <String>[
170+
'build',
171+
'impeller/compiler/code_gen_template.h',
172+
'prebuilts',
173+
'third_party',
174+
] : null,
175+
);

0 commit comments

Comments
 (0)