Refactor banned directory checking so Blink can opt into certain checks.

tools/clang/plugins/ChromeClassTester.cpp

Index: tools/clang/plugins/ChromeClassTester.cpp
diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp
index b0cc3db5aed56da1435b1ec19dff3c48fa3ff423..a2de5b9f8c93662e2464856173dffbe3ccd16f26 100644
--- a/tools/clang/plugins/ChromeClassTester.cpp
+++ b/tools/clang/plugins/ChromeClassTester.cpp
@@ -48,15 +48,15 @@ void ChromeClassTester::CheckTag(TagDecl* tag) {
   // We handle class types here where we have semantic information. We can only
   // check structs/classes/enums here, but we get a bunch of nice semantic
   // information instead of just parsing information.
+  if (InBannedNamespace(tag))

[dcheng, 2017/03/29 04:18:40]

Consolidating some common logic here.

+    return;
 
-  if (CXXRecordDecl* record = dyn_cast<CXXRecordDecl>(tag)) {
-    if (InBannedNamespace(record))
-      return;
-
-    SourceLocation record_location = record->getInnerLocStart();
-    if (InBannedDirectory(record_location))
-      return;
+  SourceLocation location = tag->getInnerLocStart();
+  LocationType location_type = ClassifyLocation(location);
+  if (location_type == LocationType::kIgnored)
+    return;
 
+  if (CXXRecordDecl* record = dyn_cast<CXXRecordDecl>(tag)) {
     // We sadly need to maintain a blacklist of types that violate these
     // rules, but do so for good reason or due to limitations of this
     // checker (i.e., we don't handle extern templates very well).
@@ -69,17 +69,14 @@ void ChromeClassTester::CheckTag(TagDecl* tag) {
     if (ends_with(base_name, "Matcher"))
         return;
 
-    CheckChromeClass(record_location, record);
+    CheckChromeClass(location_type, location, record);
   } else if (EnumDecl* enum_decl = dyn_cast<EnumDecl>(tag)) {
-    SourceLocation enum_location = enum_decl->getInnerLocStart();
-    if (InBannedDirectory(enum_location))
-      return;
-
     std::string base_name = enum_decl->getNameAsString();
+    // TODO(dcheng): This should probably consult a separate list.

[dcheng, 2017/03/29 04:18:40]

We may event want separate ignore lists for Blink vs not-Blink but that's out of
scope for now.

     if (IsIgnoredType(base_name))
       return;
 
-    CheckChromeEnum(enum_location, enum_decl);
+    CheckChromeEnum(location_type, location, enum_decl);
   }
 }
 
@@ -98,27 +95,27 @@ void ChromeClassTester::emitWarning(SourceLocation loc,
 
 }
 
-bool ChromeClassTester::InBannedDirectory(SourceLocation loc) {
+ChromeClassTester::LocationType ChromeClassTester::ClassifyLocation(
+    SourceLocation loc) {
   if (instance().getSourceManager().isInSystemHeader(loc))
-    return true;
+    return LocationType::kIgnored;
 
   std::string filename;
   if (!GetFilename(loc, &filename)) {
     // If the filename cannot be determined, simply treat this as a banned
     // location, instead of going through the full lookup process.
-    return true;
+    return LocationType::kIgnored;
   }
 
   // We need to special case scratch space; which is where clang does its
   // macro expansion. We explicitly want to allow people to do otherwise bad
   // things through macros that were defined due to third party libraries.
   if (filename == "<scratch space>")
-    return true;
+    return LocationType::kIgnored;
 
   // Don't complain about autogenerated protobuf files.
-  if (ends_with(filename, ".pb.h")) {
-    return true;
-  }
+  if (ends_with(filename, ".pb.h"))
+    return LocationType::kIgnored;
 
 #if defined(LLVM_ON_UNIX)
   // Resolve the symlinktastic relative path and make it absolute.
@@ -160,14 +157,14 @@ bool ChromeClassTester::InBannedDirectory(SourceLocation loc) {
   std::replace(filename.begin(), filename.end(), '\\', '/');
 #endif
 
-  for (const std::string& allowed_dir : allowed_directories_) {
+  for (const std::string& blink_dir : blink_directories_) {
     // If any of the allowed directories occur as a component in filename,
     // this file is allowed.
-    assert(allowed_dir.front() == '/' && "Allowed dir must start with '/'");
-    assert(allowed_dir.back() == '/' && "Allowed dir must end with '/'");
+    assert(blink_dir.front() == '/' && "Allowed dir must start with '/'");
+    assert(blink_dir.back() == '/' && "Allowed dir must end with '/'");
 
-    if (filename.find(allowed_dir) != std::string::npos)
-      return false;
+    if (filename.find(blink_dir) != std::string::npos)
+      return LocationType::kBlink;
   }
 
   for (const std::string& banned_dir : banned_directories_) {

[Nico, 2017/04/06 19:03:13]

shouldn't this be checked before blink_directories?

[dcheng, 2017/04/11 08:38:40]

third_party is in the banned list, but we don't want to classify Blink as
banned. So I think we want to still go in allowed list, banned list order of
checking.

@@ -177,24 +174,22 @@ bool ChromeClassTester::InBannedDirectory(SourceLocation loc) {
     assert(banned_dir.back() == '/' && "Banned dir must end with '/'");
 
     if (filename.find(banned_dir) != std::string::npos)
-      return true;
+      return LocationType::kIgnored;
   }
 
-  return false;
+  return LocationType::kNotIgnored;
 }
 
 bool ChromeClassTester::InBannedNamespace(const Decl* record) {
   std::string n = GetNamespace(record);
-  if (!n.empty()) {
-    return std::find(banned_namespaces_.begin(), banned_namespaces_.end(), n)
-        != banned_namespaces_.end();
-  }
+  if (!n.empty())
+    return banned_namespaces_.find(n) != banned_namespaces_.end();

[Nico, 2017/04/06 19:03:13]

:-)

 
   return false;
 }
 
 std::string ChromeClassTester::GetNamespace(const Decl* record) {
-  return GetNamespaceImpl(record->getDeclContext(), "");
+  return GetNamespaceImpl(record->getDeclContext(), std::string());
 }
 
 bool ChromeClassTester::HasIgnoredBases(const CXXRecordDecl* record) {
@@ -238,9 +233,7 @@ void ChromeClassTester::BuildBannedLists() {
   banned_namespaces_.emplace("std");
   banned_namespaces_.emplace("__gnu_cxx");
 
-  if (options_.enforce_in_thirdparty_webkit) {

[tkent, 2017/04/06 04:29:56]

We can remove Options::enforce_in_thirdparty_webkit.

[dcheng, 2017/04/06 04:41:05]

I was intentionally leaving this to provide a flag we can toggle while we
incrementally enable checks.

I guess we could try to make it more general instead (e.g. provide a flag that
lets you specify a set of checks to enable), though I'm not sure how much plugin
complexity this would add. I'll investigate this in a followup.

-    allowed_directories_.emplace("/third_party/WebKit/");
-  }
+  blink_directories_.emplace("/third_party/WebKit/");
 
   banned_directories_.emplace("/third_party/");
   banned_directories_.emplace("/native_client/");