Warn/error on "using namespace blah;" in headers on clang machines.

This adds about 25s to "make -j8" on linux. I have a few ideas on how to reduce runtime to compensate though.

BUG=carnitas
TEST=compiles

Review URL: http://codereview.chromium.org/6698051

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@78395 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 141 insertions, 47 deletions

diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp
index 74a1339..31b77b3 100644
--- a/tools/clang/plugins/ChromeClassTester.cpp
+++ b/tools/clang/plugins/ChromeClassTester.cpp
@@ -9,6 +9,8 @@
 
 #include <sys/param.h>
 
+#include "clang/Basic/FileManager.h"
+
 using namespace clang;
 
 namespace {
@@ -88,6 +90,7 @@ void ChromeClassTester::BuildBannedLists() {
 
   // Don't check autogenerated headers.
   banned_directories_.push_back("out");
+  banned_directories_.push_back("llvm");
   banned_directories_.push_back("xcodebuild");
 
   // You are standing in a mazy of twisty dependencies, all resolved by
@@ -132,6 +135,10 @@ void ChromeClassTester::BuildBannedLists() {
 ChromeClassTester::~ChromeClassTester() {}
 
 void ChromeClassTester::HandleTagDeclDefinition(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 (CXXRecordDecl* record = dyn_cast<CXXRecordDecl>(tag)) {
     // If this is a POD or a class template or a type dependent on a
     // templated class, assume there's no ctor/dtor/virtual method
@@ -165,6 +172,34 @@ void ChromeClassTester::HandleTagDeclDefinition(TagDecl* tag) {
   }
 }
 
+void ChromeClassTester::HandleTranslationUnit(clang::ASTContext& ctx) {
+  RecursivelyCheckTopLevels(ctx.getTranslationUnitDecl());
+}
+
+void ChromeClassTester::RecursivelyCheckTopLevels(Decl* d) {
+  // Unlike HandleTagDeclDefinition, we can only rely on having parsing
+  // information here. We absoluetly shouldn't check that any semantic data
+  // here because we will assert.
+
+  Decl::Kind kind = d->getKind();
+  if (kind == Decl::UsingDirective) {
+    UsingDirectiveDecl* record = cast<UsingDirectiveDecl>(d);
+    SourceLocation record_location = record->getLocation();
+    if (!InBannedDirectory(record_location))
+      CheckChromeUsingDirective(record_location, record);
+  }
+
+  // If this is a DeclContext that isn't a function/method, recurse.
+  if (DeclContext* context = dyn_cast<DeclContext>(d)) {
+    if (context->isFileContext()) {
+      for (DeclContext::decl_iterator it = context->decls_begin();
+           it != context->decls_end(); ++it) {
+        RecursivelyCheckTopLevels(*it);
+      }
+    }
+  }
+}
+
 void ChromeClassTester::emitWarning(SourceLocation loc, const char* raw_error) {
   FullSourceLoc full(loc, instance().getSourceManager());
   std::string err;
@@ -219,40 +254,51 @@ std::string ChromeClassTester::GetNamespaceImpl(const DeclContext* context,
   }
 }
 
-bool ChromeClassTester::InBannedDirectory(const SourceLocation& loc) {
-  if (loc.isFileID() && loc.isValid()) {
-    bool invalid = false;
-    const char* buffer_name = instance_.getSourceManager().getBufferName(
-        loc, &invalid);
-    if (!invalid && buffer_name) {
-      std::string b(buffer_name);
+bool ChromeClassTester::InBannedDirectory(SourceLocation loc) {
+  const SourceManager &SM = instance_.getSourceManager();
+  SourceLocation spelling_location = SM.getSpellingLoc(loc);
+  PresumedLoc ploc = SM.getPresumedLoc(spelling_location);
+  std::string buffer_name;
+  if (ploc.isInvalid()) {
+    // If we're in an invalid location, we're looking at things that aren't
+    // actually stated in the source; treat this as a banned location instead
+    // of going through our full lookup process.
+    return true;
+  } else {
+    std::string b = ploc.getFilename();
+
+    // 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 (b == "<scratch space>")
+      return true;
 
-      // Don't complain about these things in implementation files.
-      if (ends_with(b, ".cc") || ends_with(b, ".cpp") || ends_with(b, ".mm")) {
-        return true;
-      }
+    // Don't complain about these things in implementation files.
+    if (ends_with(b, ".cc") || ends_with(b, ".cpp") || ends_with(b, ".mm")) {
+      return true;
+    }
 
-      // Don't complain about autogenerated protobuf files.
-      if (ends_with(b, ".pb.h")) {
-        return true;
-      }
+    // Don't complain about autogenerated protobuf files.
+    if (ends_with(b, ".pb.h")) {
+      return true;
+    }
 
-      // We need to munge the paths so that they are relative to the repository
-      // srcroot. We first resolve the symlinktastic relative path and then
-      // remove our known srcroot from it if needed.
-      char resolvedPath[MAXPATHLEN];
-      if (realpath(b.c_str(), resolvedPath)) {
-        std::string resolved = resolvedPath;
-        if (starts_with(resolved, src_root_)) {
-          b = resolved.substr(src_root_.size());
-        }
+    // We need to munge the paths so that they are relative to the repository
+    // srcroot. We first resolve the symlinktastic relative path and then
+    // remove our known srcroot from it if needed.
+    char resolvedPath[MAXPATHLEN];
+    if (realpath(b.c_str(), resolvedPath)) {
+      std::string resolved = resolvedPath;
+      if (starts_with(resolved, src_root_)) {
+        b = resolved.substr(src_root_.size());
       }
+    }
 
-      for (std::vector<std::string>::const_iterator it =
-               banned_directories_.begin();
-           it != banned_directories_.end(); ++it) {
-        if (starts_with(b, *it))
-          return true;
+    for (std::vector<std::string>::const_iterator it =
+             banned_directories_.begin();
+         it != banned_directories_.end(); ++it) {
+      if (starts_with(b, *it)) {
+        return true;
       }
     }
   }
diff --git a/tools/clang/plugins/ChromeClassTester.h b/tools/clang/plugins/ChromeClassTester.h
index f755c1c..23d9715 100644
--- a/tools/clang/plugins/ChromeClassTester.h
+++ b/tools/clang/plugins/ChromeClassTester.h
@@ -25,6 +25,7 @@ class ChromeClassTester : public clang::ASTConsumer {
 
   // ASTConsumer:
   virtual void HandleTagDeclDefinition(clang::TagDecl* tag);
+  virtual void HandleTranslationUnit(clang::ASTContext& ctx);
 
  protected:
   clang::CompilerInstance& instance() { return instance_; }
@@ -40,10 +41,17 @@ class ChromeClassTester : public clang::ASTConsumer {
   bool InTestingNamespace(clang::Decl* record);
 
  private:
-  // Template method which is called with only classes that are defined in
+  // Filtered versions of tags that are only called with things defined in
   // chrome header files.
   virtual void CheckChromeClass(const clang::SourceLocation& record_location,
                                 clang::CXXRecordDecl* record) = 0;
+  virtual void CheckChromeUsingDirective(
+      const clang::SourceLocation& record_location,
+      clang::UsingDirectiveDecl* record) = 0;
+
+  // The ChromeClassTester receives each top level declaration, looking for
+  // Decls we are interested in.
+  void RecursivelyCheckTopLevels(clang::Decl* d);
 
   // Utility methods used for filtering out non-chrome classes (and ones we
   // delibrately ignore) in HandleTagDeclDefinition().
@@ -51,7 +59,7 @@ class ChromeClassTester : public clang::ASTConsumer {
   std::string GetNamespace(clang::Decl* record);
   std::string GetNamespaceImpl(const clang::DeclContext* context,
                                std::string candidate);
-  bool InBannedDirectory(const clang::SourceLocation& loc);
+  bool InBannedDirectory(clang::SourceLocation loc);
   bool IsIgnoredType(const std::string& base_name);
 
   clang::CompilerInstance& instance_;
diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp
index d7bcdae..3c8f623 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -47,6 +47,16 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     CheckVirtualMethods(record_location, record);
   }
 
+  virtual void CheckChromeUsingDirective(
+      const clang::SourceLocation& record_location,
+      clang::UsingDirectiveDecl* record) {
+    // |record| is a using directive in a chrome header. This should never be
+    // allowed.
+    emitWarning(record_location,
+                "Using namespace directives are banned in headers in Google "
+                "style.");
+  }
+
   // Prints errors if the constructor/destructor weight it too heavy.
   void CheckCtorDtorWeight(const SourceLocation& record_location,
                            CXXRecordDecl* record) {
diff --git a/tools/clang/plugins/tests/using_directive.cpp b/tools/clang/plugins/tests/using_directive.cpp
new file mode 100644
index 0000000..f48f2c1
--- /dev/null
+++ b/tools/clang/plugins/tests/using_directive.cpp
@@ -0,0 +1,15 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "using_directive.h"
+
+int main() {
+  // Don't warn here. We use this pattern in unit tests and it's probably
+  // harmless. Headers included in multiple translation units could be harmful
+  // though.
+  using namespace std;
+  vector<int> a;
+  a.push_back(1);
+  return 0;
+}
diff --git a/tools/clang/plugins/tests/using_directive.h b/tools/clang/plugins/tests/using_directive.h
new file mode 100644
index 0000000..d31840f
--- /dev/null
+++ b/tools/clang/plugins/tests/using_directive.h
@@ -0,0 +1,13 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef USING_DIRECTIVE_H_
+#define USING_DIRECTIVE_H_
+
+#include <vector>
+
+// Should warn in headers.
+using namespace std;
+
+#endif  // USING_DIRECTIVE_H_
diff --git a/tools/clang/plugins/tests/using_directive.txt b/tools/clang/plugins/tests/using_directive.txt
new file mode 100644
index 0000000..d415fa5
--- /dev/null
+++ b/tools/clang/plugins/tests/using_directive.txt
@@ -0,0 +1,5 @@
+In file included from using_directive.cpp:5:
+./using_directive.h:11:17: warning: [chromium-style] Using namespace directives are banned in headers in Google style.
+using namespace std;
+                ^
+1 warning generated.
diff --git a/ui/gfx/gtk_util.h b/ui/gfx/gtk_util.h
index 71cb792..fd3575f 100644
--- a/ui/gfx/gtk_util.h
+++ b/ui/gfx/gtk_util.h
@@ -61,23 +61,21 @@ uint8_t* BGRAToRGBA(const uint8_t* pixels, int width, int height, int stride);
 
 }  // namespace gfx
 
-namespace {
-// A helper class that will g_object_unref |p| when it goes out of scope.
-// This never adds a ref, it only unrefs.
-template <typename Type>
-struct GObjectUnrefer {
-  void operator()(Type* ptr) const {
-    if (ptr)
-      g_object_unref(ptr);
-  }
-};
-}  // namespace
-
 // It's not legal C++ to have a templatized typedefs, so we wrap it in a
 // struct.  When using this, you need to include ::Type.  E.g.,
 // ScopedGObject<GdkPixbufLoader>::Type loader(gdk_pixbuf_loader_new());
 template<class T>
 struct ScopedGObject {
+  // A helper class that will g_object_unref |p| when it goes out of scope.
+  // This never adds a ref, it only unrefs.
+  template<class U>
+  struct GObjectUnrefer {
+    void operator()(U* ptr) const {
+      if (ptr)
+        g_object_unref(ptr);
+    }
+  };
+
   typedef scoped_ptr_malloc<T, GObjectUnrefer<T> > Type;
 };
 
diff --git a/webkit/glue/webvideoframe_impl.h b/webkit/glue/webvideoframe_impl.h
index 0aca052..52f7f1f 100644
--- a/webkit/glue/webvideoframe_impl.h
+++ b/webkit/glue/webvideoframe_impl.h
@@ -8,14 +8,13 @@
 #include "media/base/video_frame.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebVideoFrame.h"
 
-using namespace WebKit;
-
 namespace webkit_glue {
 
-class WebVideoFrameImpl : public WebVideoFrame {
+class WebVideoFrameImpl : public WebKit::WebVideoFrame {
  public:
   // This converts a WebKit::WebVideoFrame to a media::VideoFrame.
-  static media::VideoFrame* toVideoFrame(WebVideoFrame* web_video_frame);
+  static media::VideoFrame* toVideoFrame(
+      WebKit::WebVideoFrame* web_video_frame);
 
   WebVideoFrameImpl(scoped_refptr<media::VideoFrame> video_frame);
   virtual ~WebVideoFrameImpl();