Improved llvm-namespace-comment check.

Summary:
Handle various forms of existing namespace closing comments, fix
existing comments with wrong namespace name, ignore short namespaces.

The state of this check now seems to be enough to enable it by default to gather
user feedback ;)

Reviewers: klimek

Reviewed By: klimek

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D3825

llvm-svn: 209141

8 files changed, 277 insertions, 76 deletions

diff --git a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
index cf9baaef7e9..e3314d8eef3 100644
--- a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
@@ -1,7 +1,9 @@
 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyLLVMModule
+  IncludeOrderCheck.cpp
   LLVMTidyModule.cpp
+  NamespaceCommentCheck.cpp
 
   LINK_LIBS
   clangAST
diff --git a/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp b/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp
new file mode 100644
index 00000000000..de4f650a33c
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp
@@ -0,0 +1,44 @@
+//===--- IncludeOrderCheck.cpp - clang-tidy -------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "IncludeOrderCheck.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+class IncludeOrderPPCallbacks : public PPCallbacks {
+public:
+  explicit IncludeOrderPPCallbacks(IncludeOrderCheck &Check) : Check(Check) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+                          StringRef FileName, bool IsAngled,
+                          CharSourceRange FilenameRange, const FileEntry *File,
+                          StringRef SearchPath, StringRef RelativePath,
+                          const Module *Imported) override {
+    // FIXME: This is a dummy implementation to show how to get at preprocessor
+    // information. Implement a real include order check.
+    Check.diag(HashLoc, "This is an include");
+  }
+
+private:
+  IncludeOrderCheck &Check;
+};
+} // namespace
+
+void IncludeOrderCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+  Compiler.getPreprocessor()
+      .addPPCallbacks(new IncludeOrderPPCallbacks(*this));
+}
+
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.h b/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.h
new file mode 100644
index 00000000000..ba77c947644
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.h
@@ -0,0 +1,29 @@
+//===--- IncludeOrderCheck.h - clang-tidy -----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_INCLUDE_ORDER_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_INCLUDE_ORDER_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Checks the correct order of \c #includes.
+///
+/// see: http://llvm.org/docs/CodingStandards.html#include-style
+class IncludeOrderCheck : public ClangTidyCheck {
+public:
+  void registerPPCallbacks(CompilerInstance &Compiler) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_INCLUDE_ORDER_CHECK_H
diff --git a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
index 32766731586..31a114eeebd 100644
--- a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
@@ -7,75 +7,15 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "LLVMTidyModule.h"
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
-#include "clang/AST/ASTContext.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Frontend/CompilerInstance.h"
-#include "clang/Lex/PPCallbacks.h"
-#include "clang/Lex/Preprocessor.h"
-#include "llvm/Support/raw_ostream.h"
-
-using namespace clang::ast_matchers;
+#include "IncludeOrderCheck.h"
+#include "NamespaceCommentCheck.h"
 
 namespace clang {
 namespace tidy {
 
-void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(namespaceDecl().bind("namespace"), this);
-}
-
-void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
-  const NamespaceDecl *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
-  Token Tok;
-  SourceLocation Loc = ND->getRBraceLoc().getLocWithOffset(1);
-  while (Lexer::getRawToken(Loc, Tok, *Result.SourceManager,
-                            Result.Context->getLangOpts())) {
-    Loc = Loc.getLocWithOffset(1);
-  }
-  // FIXME: Check that this namespace is "long".
-  if (Tok.is(tok::comment)) {
-    // FIXME: Check comment content.
-    // FIXME: Check comment placement on the same line.
-    return;
-  }
-  std::string Fix = " // namespace";
-  if (!ND->isAnonymousNamespace())
-    Fix = Fix.append(" ").append(ND->getNameAsString());
-
-  diag(ND->getLocation(), "namespace not terminated with a closing comment")
-      << FixItHint::CreateInsertion(ND->getRBraceLoc().getLocWithOffset(1),
-                                    Fix);
-}
-
-namespace {
-class IncludeOrderPPCallbacks : public PPCallbacks {
-public:
-  explicit IncludeOrderPPCallbacks(IncludeOrderCheck &Check) : Check(Check) {}
-
-  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
-                          StringRef FileName, bool IsAngled,
-                          CharSourceRange FilenameRange, const FileEntry *File,
-                          StringRef SearchPath, StringRef RelativePath,
-                          const Module *Imported) override {
-    // FIXME: This is a dummy implementation to show how to get at preprocessor
-    // information. Implement a real include order check.
-    Check.diag(HashLoc, "This is an include");
-  }
-
-private:
-  IncludeOrderCheck &Check;
-};
-} // namespace
-
-void IncludeOrderCheck::registerPPCallbacks(CompilerInstance &Compiler) {
-  Compiler.getPreprocessor()
-      .addPPCallbacks(new IncludeOrderPPCallbacks(*this));
-}
-
 class LLVMModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
diff --git a/clang-tools-extra/clang-tidy/llvm/NamespaceCommentCheck.cpp b/clang-tools-extra/clang-tidy/llvm/NamespaceCommentCheck.cpp
new file mode 100644
index 00000000000..b5dca4ba13f
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/llvm/NamespaceCommentCheck.cpp
@@ -0,0 +1,115 @@
+//===--- NamespaceCommentCheck.cpp - clang-tidy ---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "NamespaceCommentCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
+
+
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+NamespaceCommentCheck::NamespaceCommentCheck()
+    : NamespaceCommentPattern("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
+                              "namespace( +([a-zA-Z0-9_]+))? *(\\*/)?$",
+                              llvm::Regex::IgnoreCase),
+      ShortNamespaceLines(1) {}
+
+void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(namespaceDecl().bind("namespace"), this);
+}
+
+bool locationsInSameFile(const SourceManager &Sources, SourceLocation Loc1,
+                         SourceLocation Loc2) {
+  return Loc1.isFileID() && Loc2.isFileID() &&
+         Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
+}
+
+std::string getNamespaceComment(const NamespaceDecl *ND, bool InsertLineBreak) {
+  std::string Fix = "// namespace";
+  if (!ND->isAnonymousNamespace())
+    Fix.append(" ").append(ND->getNameAsString());
+  if (InsertLineBreak)
+    Fix.append("\n");
+  return Fix;
+}
+
+void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
+  const NamespaceDecl *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
+  const SourceManager &Sources = *Result.SourceManager;
+
+  if (!locationsInSameFile(Sources, ND->getLocStart(), ND->getRBraceLoc()))
+    return;
+
+  // Don't require closing comments for namespaces spanning less than certain
+  // number of lines.
+  unsigned StartLine = Sources.getSpellingLineNumber(ND->getLocStart());
+  unsigned EndLine = Sources.getSpellingLineNumber(ND->getRBraceLoc());
+  if (EndLine - StartLine + 1 <= ShortNamespaceLines)
+    return;
+
+  // Find next token after the namespace closing brace.
+  SourceLocation AfterRBrace = ND->getRBraceLoc().getLocWithOffset(1);
+  SourceLocation Loc = AfterRBrace;
+  Token Tok;
+  // Skip whitespace until we find the next token.
+  while (Lexer::getRawToken(Loc, Tok, Sources, Result.Context->getLangOpts())) {
+    Loc = Loc.getLocWithOffset(1);
+  }
+  if (!locationsInSameFile(Sources, ND->getRBraceLoc(), Loc))
+    return;
+
+  bool NextTokenIsOnSameLine = Sources.getSpellingLineNumber(Loc) == EndLine;
+  bool NeedLineBreak = NextTokenIsOnSameLine && Tok.isNot(tok::eof);
+
+  // Try to find existing namespace closing comment on the same line.
+  if (Tok.is(tok::comment) && NextTokenIsOnSameLine) {
+    StringRef Comment(Sources.getCharacterData(Loc), Tok.getLength());
+    SmallVector<StringRef, 6> Groups;
+    if (NamespaceCommentPattern.match(Comment, &Groups)) {
+      StringRef NamespaceNameInComment = Groups.size() >= 6 ? Groups[5] : "";
+
+      // Check if the namespace in the comment is the same.
+      if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) ||
+          ND->getNameAsString() == NamespaceNameInComment) {
+        // FIXME: Maybe we need a strict mode, where we always fix namespace
+        // comments with different format.
+        return;
+      }
+
+      // Otherwise we need to fix the comment.
+      NeedLineBreak = Comment.startswith("/*");
+      CharSourceRange OldCommentRange = CharSourceRange::getCharRange(
+          SourceRange(Loc, Loc.getLocWithOffset(Tok.getLength())));
+      diag(Loc, "namespace closing comment refers to a wrong namespace '%0'")
+          << NamespaceNameInComment
+          << FixItHint::CreateReplacement(
+                 OldCommentRange, getNamespaceComment(ND, NeedLineBreak));
+      return;
+    }
+
+    // This is not a recognized form of a namespace closing comment.
+    // Leave line comment on the same line. Move block comment to the next line,
+    // as it can be multi-line or there may be other tokens behind it.
+    if (Comment.startswith("//"))
+      NeedLineBreak = false;
+  }
+
+  diag(ND->getLocation(), "namespace not terminated with a closing comment")
+      << FixItHint::CreateInsertion(
+          AfterRBrace, " " + getNamespaceComment(ND, NeedLineBreak));
+}
+
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.h b/clang-tools-extra/clang-tidy/llvm/NamespaceCommentCheck.h
index b619a5e00b2..87932807d43 100644
--- a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.h
+++ b/clang-tools-extra/clang-tidy/llvm/NamespaceCommentCheck.h
@@ -1,4 +1,4 @@
-//===--- LLVMTidyModule.h - clang-tidy --------------------------*- C++ -*-===//
+//===--- NamespaceCommentCheck.h - clang-tidy -------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -7,32 +7,30 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_LLVM_TIDY_MODULE_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_LLVM_TIDY_MODULE_H
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_NAMESPACE_COMMENT_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_NAMESPACE_COMMENT_CHECK_H
 
 #include "../ClangTidy.h"
+#include "llvm/Support/Regex.h"
 
 namespace clang {
 namespace tidy {
 
-/// \brief Checks the correct order of \c #includes.
-///
-/// see: http://llvm.org/docs/CodingStandards.html#include-style
-class IncludeOrderCheck : public ClangTidyCheck {
-public:
-  void registerPPCallbacks(CompilerInstance &Compiler) override;
-};
-
 /// \brief Checks that long namespaces have a closing comment.
 ///
 /// see: http://llvm.org/docs/CodingStandards.html#namespace-indentation
 class NamespaceCommentCheck : public ClangTidyCheck {
 public:
+  NamespaceCommentCheck();
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  llvm::Regex NamespaceCommentPattern;
+  const unsigned ShortNamespaceLines;
 };
 
 } // namespace tidy
 } // namespace clang
 
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_TIDY_LLVM_MODULE_H
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_NAMESPACE_COMMENT_CHECK_H
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index 86def378e5b..a43af88c027 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -31,7 +31,6 @@ const char DefaultChecks[] =
     "*,"                       // Enable all checks, except these:
     "-clang-analyzer-alpha*,"  // Too many false positives.
     "-llvm-include-order,"     // Not implemented yet.
-    "-llvm-namespace-comment," // Not complete.
     "-google-*,";              // Doesn't apply to LLVM.
 static cl::opt<std::string>
 Checks("checks", cl::desc("Comma-separated list of globs with optional '-'\n"
diff --git a/clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp b/clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
index e69616499c0..6bc52b23188 100644
--- a/clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -1,5 +1,6 @@
 #include "ClangTidyTest.h"
-#include "llvm/LLVMTidyModule.h"
+#include "llvm/IncludeOrderCheck.h"
+#include "llvm/NamespaceCommentCheck.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -9,6 +10,79 @@ namespace test {
 TEST(NamespaceCommentCheckTest, Basic) {
   EXPECT_EQ("namespace i {\n} // namespace i",
             runCheckOnCode<NamespaceCommentCheck>("namespace i {\n}"));
+  EXPECT_EQ("namespace {\n} // namespace",
+            runCheckOnCode<NamespaceCommentCheck>("namespace {\n}"));
+  EXPECT_EQ(
+      "namespace i { namespace j {\n} // namespace j\n } // namespace i",
+      runCheckOnCode<NamespaceCommentCheck>("namespace i { namespace j {\n} }"));
+}
+
+TEST(NamespaceCommentCheckTest, SingleLineNamespaces) {
+  EXPECT_EQ(
+      "namespace i { namespace j { } }",
+      runCheckOnCode<NamespaceCommentCheck>("namespace i { namespace j { } }"));
+}
+
+TEST(NamespaceCommentCheckTest, CheckExistingComments) {
+  EXPECT_EQ("namespace i { namespace j {\n"
+            "} /* namespace j */ } // namespace i\n"
+            " /* random comment */",
+            runCheckOnCode<NamespaceCommentCheck>(
+                "namespace i { namespace j {\n"
+                "} /* namespace j */ } /* random comment */"));
+  EXPECT_EQ("namespace {\n"
+            "} // namespace",
+            runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
+                                                  "} // namespace"));
+  EXPECT_EQ("namespace {\n"
+            "} //namespace",
+            runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
+                                                  "} //namespace"));
+  EXPECT_EQ("namespace {\n"
+            "} // anonymous namespace",
+            runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
+                                                  "} // anonymous namespace"));
+  EXPECT_EQ(
+      "namespace My_NameSpace123 {\n"
+      "} // namespace My_NameSpace123",
+      runCheckOnCode<NamespaceCommentCheck>("namespace My_NameSpace123 {\n"
+                                            "} // namespace My_NameSpace123"));
+  EXPECT_EQ(
+      "namespace My_NameSpace123 {\n"
+      "} //namespace My_NameSpace123",
+      runCheckOnCode<NamespaceCommentCheck>("namespace My_NameSpace123 {\n"
+                                            "} //namespace My_NameSpace123"));
+  EXPECT_EQ("namespace My_NameSpace123 {\n"
+            "} //  end namespace   My_NameSpace123",
+            runCheckOnCode<NamespaceCommentCheck>(
+                "namespace My_NameSpace123 {\n"
+                "} //  end namespace   My_NameSpace123"));
+  // Understand comments only on the same line.
+  EXPECT_EQ("namespace {\n"
+            "} // namespace\n"
+            "// namespace",
+            runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
+                                                  "}\n"
+                                                  "// namespace"));
+  // Leave unknown comments.
+  EXPECT_EQ("namespace {\n"
+            "} // namespace // random text",
+            runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
+                                                  "} // random text"));
+}
+
+TEST(NamespaceCommentCheckTest, FixWrongComments) {
+  EXPECT_EQ("namespace i { namespace jJ0_ {\n"
+            "} // namespace jJ0_\n"
+            " } // namespace i\n"
+            " /* random comment */",
+            runCheckOnCode<NamespaceCommentCheck>(
+                "namespace i { namespace jJ0_ {\n"
+                "} /* namespace qqq */ } /* random comment */"));
+  EXPECT_EQ("namespace {\n"
+            "} // namespace",
+            runCheckOnCode<NamespaceCommentCheck>("namespace {\n"
+                                                  "} // namespace asdf"));
 }
 
 } // namespace test