Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(10)

Side by Side Diff: tools/gn/header_checker.cc

Issue 231813002: Improve GN public header file checking (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Created 6 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch | Annotate | Revision Log
« no previous file with comments | « tools/gn/header_checker.h ('k') | tools/gn/header_checker_unittest.cc » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
1 // Copyright 2014 The Chromium Authors. All rights reserved. 1 // Copyright 2014 The Chromium Authors. All rights reserved.
2 // Use of this source code is governed by a BSD-style license that can be 2 // Use of this source code is governed by a BSD-style license that can be
3 // found in the LICENSE file. 3 // found in the LICENSE file.
4 4
5 #include "tools/gn/header_checker.h" 5 #include "tools/gn/header_checker.h"
6 6
7 #include <algorithm> 7 #include <algorithm>
8 8
9 #include "base/bind.h" 9 #include "base/bind.h"
10 #include "base/file_util.h" 10 #include "base/file_util.h"
11 #include "base/message_loop/message_loop.h" 11 #include "base/message_loop/message_loop.h"
12 #include "base/threading/sequenced_worker_pool.h" 12 #include "base/threading/sequenced_worker_pool.h"
13 #include "tools/gn/build_settings.h" 13 #include "tools/gn/build_settings.h"
14 #include "tools/gn/builder.h" 14 #include "tools/gn/builder.h"
15 #include "tools/gn/c_include_iterator.h" 15 #include "tools/gn/c_include_iterator.h"
16 #include "tools/gn/err.h" 16 #include "tools/gn/err.h"
17 #include "tools/gn/filesystem_utils.h" 17 #include "tools/gn/filesystem_utils.h"
18 #include "tools/gn/scheduler.h" 18 #include "tools/gn/scheduler.h"
19 #include "tools/gn/target.h" 19 #include "tools/gn/target.h"
20 #include "tools/gn/trace.h" 20 #include "tools/gn/trace.h"
21 21
22 namespace {
23
24 // This class makes InputFiles on the stack as it reads files to check. When
25 // we throw an error, the Err indicates a locatin which has a pointer to
26 // an InputFile that must persist as long as the Err does.
27 //
28 // To make this work, this function creates a clone of the InputFile managed
29 // by the InputFileManager so the error can refer to something that
30 // persists. This means that the current file contents will live as long as
31 // the program, but this is OK since we're erroring out anyway.
32 LocationRange CreatePersistentRange(const InputFile& input_file,
33 const LocationRange& range) {
34 InputFile* clone_input_file;
35 std::vector<Token>* tokens; // Don't care about this.
36 scoped_ptr<ParseNode>* parse_root; // Don't care about this.
37
38 g_scheduler->input_file_manager()->AddDynamicInput(
39 input_file.name(), &clone_input_file, &tokens, &parse_root);
40 clone_input_file->SetContents(input_file.contents());
41
42 return LocationRange(
43 Location(clone_input_file, range.begin().line_number(),
44 range.begin().char_offset()),
45 Location(clone_input_file, range.end().line_number(),
46 range.end().char_offset()));
47 }
48
49 } // namespace
50
22 HeaderChecker::HeaderChecker(const BuildSettings* build_settings, 51 HeaderChecker::HeaderChecker(const BuildSettings* build_settings,
23 const std::vector<const Target*>& targets) 52 const std::vector<const Target*>& targets)
24 : main_loop_(base::MessageLoop::current()), 53 : main_loop_(base::MessageLoop::current()),
25 build_settings_(build_settings) { 54 build_settings_(build_settings) {
26 for (size_t i = 0; i < targets.size(); i++) 55 for (size_t i = 0; i < targets.size(); i++)
27 AddTargetToFileMap(targets[i]); 56 AddTargetToFileMap(targets[i]);
28 } 57 }
29 58
30 HeaderChecker::~HeaderChecker() { 59 HeaderChecker::~HeaderChecker() {
31 } 60 }
(...skipping 98 matching lines...) Expand 10 before | Expand all | Expand 10 after
130 159
131 base::FilePath path = build_settings_->GetFullPath(file); 160 base::FilePath path = build_settings_->GetFullPath(file);
132 std::string contents; 161 std::string contents;
133 if (!base::ReadFileToString(path, &contents)) { 162 if (!base::ReadFileToString(path, &contents)) {
134 *err = Err(from_target->defined_from(), "Source file not found.", 163 *err = Err(from_target->defined_from(), "Source file not found.",
135 "This target includes as a source:\n " + file.value() + 164 "This target includes as a source:\n " + file.value() +
136 "\nwhich was not found."); 165 "\nwhich was not found.");
137 return false; 166 return false;
138 } 167 }
139 168
140 CIncludeIterator iter(contents); 169 InputFile input_file(file);
170 input_file.SetContents(contents);
171
172 CIncludeIterator iter(&input_file);
141 base::StringPiece current_include; 173 base::StringPiece current_include;
142 while (iter.GetNextIncludeString(&current_include)) { 174 LocationRange range;
175 while (iter.GetNextIncludeString(&current_include, &range)) {
143 SourceFile include = SourceFileForInclude(current_include); 176 SourceFile include = SourceFileForInclude(current_include);
144 if (!CheckInclude(from_target, file, include, err)) 177 if (!CheckInclude(from_target, input_file, include, range, err))
145 return false; 178 return false;
146 } 179 }
147 180
148 return true; 181 return true;
149 } 182 }
150 183
151 // If the file exists, it must be in a dependency of the given target, and it 184 // If the file exists, it must be in a dependency of the given target, and it
152 // must be public in that dependency. 185 // must be public in that dependency.
153 bool HeaderChecker::CheckInclude(const Target* from_target, 186 bool HeaderChecker::CheckInclude(const Target* from_target,
154 const SourceFile& source_file, 187 const InputFile& source_file,
155 const SourceFile& include_file, 188 const SourceFile& include_file,
189 const LocationRange& range,
156 Err* err) const { 190 Err* err) const {
157 // Assume if the file isn't declared in our sources that we don't need to 191 // Assume if the file isn't declared in our sources that we don't need to
158 // check it. It would be nice if we could give an error if this happens, but 192 // check it. It would be nice if we could give an error if this happens, but
159 // our include finder is too primitive and returns all includes, even if 193 // our include finder is too primitive and returns all includes, even if
160 // they're in a #if not executed in the current build. In that case, it's 194 // they're in a #if not executed in the current build. In that case, it's
161 // not unusual for the buildfiles to not specify that header at all. 195 // not unusual for the buildfiles to not specify that header at all.
162 FileMap::const_iterator found = file_map_.find(include_file); 196 FileMap::const_iterator found = file_map_.find(include_file);
163 if (found == file_map_.end()) 197 if (found == file_map_.end())
164 return true; 198 return true;
165 199
166 const TargetVector& targets = found->second; 200 const TargetVector& targets = found->second;
167 201
168 // For all targets containing this file, we require that at least one be 202 // For all targets containing this file, we require that at least one be
169 // a dependency of the current target, and all targets that are dependencies 203 // a dependency of the current target, and all targets that are dependencies
170 // must have the file listed in the public section. 204 // must have the file listed in the public section.
171 bool found_dependency = false; 205 bool found_dependency = false;
172 for (size_t i = 0; i < targets.size(); i++) { 206 for (size_t i = 0; i < targets.size(); i++) {
173 // We always allow source files in a target to include headers also in that 207 // We always allow source files in a target to include headers also in that
174 // target. 208 // target.
175 if (targets[i].target == from_target) 209 if (targets[i].target == from_target)
176 return true; 210 return true;
177 211
178 if (IsDependencyOf(targets[i].target, from_target)) { 212 if (IsDependencyOf(targets[i].target, from_target)) {
179 // The include is in a target that's a proper dependency. Now verify 213 // The include is in a target that's a proper dependency. Verify that
180 // that the include is a public file. 214 // the including target has visibility.
215 if (!targets[i].target->visibility().CanSeeMe(from_target->label())) {
216 std::string msg = "The included file is in " +
217 targets[i].target->label().GetUserVisibleName(false) +
218 "\nwhich is not visible from " +
219 from_target->label().GetUserVisibleName(false) +
220 "\n(see \"gn help visibility\").";
221
222 // Danger: must call CreatePersistentRange to put in Err.
223 *err = Err(CreatePersistentRange(source_file, range),
224 "Including a header from non-visible target.", msg);
225 return false;
226 }
227
228 // The file must also be public in the target.
181 if (!targets[i].is_public) { 229 if (!targets[i].is_public) {
182 // Depending on a private header. 230 // Danger: must call CreatePersistentRange to put in Err.
183 std::string msg = "The file " + source_file.value() + 231 *err = Err(CreatePersistentRange(source_file, range),
184 "\nincludes " + include_file.value() + 232 "Including a private header.",
185 "\nwhich is private to the target " + 233 "This file is private to the target " +
186 targets[i].target->label().GetUserVisibleName(false); 234 targets[i].target->label().GetUserVisibleName(false));
187
188 // TODO(brettw) blame the including file.
189 *err = Err(NULL, "Including a private header.", msg);
190 return false; 235 return false;
191 } 236 }
192 found_dependency = true; 237 found_dependency = true;
193 } 238 }
194 } 239 }
195 240
196 if (!found_dependency) { 241 if (!found_dependency) {
197 std::string msg = 242 std::string msg = "It is not in any dependency of " +
198 source_file.value() + " includes the header\n" +
199 include_file.value() + " which is not in any dependency of\n" +
200 from_target->label().GetUserVisibleName(false); 243 from_target->label().GetUserVisibleName(false);
201 msg += "\n\nThe include file is in the target(s):\n"; 244 msg += "\nThe include file is in the target(s):\n";
202 for (size_t i = 0; i < targets.size(); i++) 245 for (size_t i = 0; i < targets.size(); i++)
203 msg += " " + targets[i].target->label().GetUserVisibleName(false) + "\n"; 246 msg += " " + targets[i].target->label().GetUserVisibleName(false) + "\n";
247 if (targets.size() > 1)
248 msg += "at least one of ";
249 msg += "which should somehow be reachable from " +
250 from_target->label().GetUserVisibleName(false);
204 251
205 msg += "\nMake sure one of these is a direct or indirect dependency\n" 252 // Danger: must call CreatePersistentRange to put in Err.
206 "of " + from_target->label().GetUserVisibleName(false); 253 *err = Err(CreatePersistentRange(source_file, range),
207 254 "Include not allowed.", msg);
208 // TODO(brettw) blame the including file.
209 // Probably this means making and leaking an input file for it, and also
210 // tracking the locations for each include.
211 *err = Err(NULL, "Include not allowed.", msg);
212 return false; 255 return false;
213 } 256 }
214 257
215 return true; 258 return true;
216 } 259 }
217 260
218 bool HeaderChecker::IsDependencyOf(const Target* search_for, 261 bool HeaderChecker::IsDependencyOf(const Target* search_for,
219 const Target* search_from) const { 262 const Target* search_from) const {
220 std::set<const Target*> checked; 263 std::set<const Target*> checked;
221 return IsDependencyOf(search_for, search_from, &checked); 264 return IsDependencyOf(search_for, search_from, &checked);
(...skipping 11 matching lines...) Expand all
233 return true; // Found it. 276 return true; // Found it.
234 277
235 // Recursive search. 278 // Recursive search.
236 checked->insert(deps[i].ptr); 279 checked->insert(deps[i].ptr);
237 if (IsDependencyOf(search_for, deps[i].ptr, checked)) 280 if (IsDependencyOf(search_for, deps[i].ptr, checked))
238 return true; 281 return true;
239 } 282 }
240 283
241 return false; 284 return false;
242 } 285 }
OLDNEW
« no previous file with comments | « tools/gn/header_checker.h ('k') | tools/gn/header_checker_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698