Improved llvm-namespace-comment check.

[alexfh]

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 ;)

[klimek]

clang-tidy/llvm/NamespaceCommentCheck.cpp

43

This overall needs a lot more comments to be understandable... perhaps also try to pull out a few functions?

[alexfh]

Added comments and pulled out a couple of functions. PTAL.

[klimek]

lg

clang-tidy/llvm/NamespaceCommentCheck.cpp

67

Can you add a comment explaining why it's ok to retry on failure with an incremented location?

74

Can you explain why we need this? (I'd have expected the fix-it to not change the number of newlines, thus just inserting the comment between the } and the newline).

[alexfh]

Added a comment.

clang-tidy/llvm/NamespaceCommentCheck.cpp

67

Done.

74

We need it for the case where the next token after '}' is on the same line (see the test cases on lines 17 and 32 of unittests/clang-tidy/LLVMModuleTest.cpp).

clang-tidy/llvm/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyLLVMModule
+  IncludeOrderCheck.cpp
   LLVMTidyModule.cpp
+  NamespaceCommentCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/llvm/IncludeOrderCheck.h

+//===--- 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

clang-tidy/llvm/IncludeOrderCheck.cpp

+//===--- 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

clang-tidy/llvm/LLVMTidyModule.h

-//===--- LLVMTidyModule.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_LLVM_TIDY_MODULE_H
-#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_LLVM_TIDY_MODULE_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;
-};
-
-/// \brief Checks that long namespaces have a closing comment.
-///
-/// see: http://llvm.org/docs/CodingStandards.html#namespace-indentation
-class NamespaceCommentCheck : public ClangTidyCheck {
-public:
-  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
-  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
-};
-
-} // namespace tidy
-} // namespace clang
-
-#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_TIDY_LLVM_MODULE_H

clang-tidy/llvm/LLVMTidyModule.cpp

 //===--- LLVMTidyModule.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 "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 {
     CheckFactories.addCheckFactory(
         "llvm-include-order", new ClangTidyCheckFactory<IncludeOrderCheck>());
     CheckFactories.addCheckFactory(
         "llvm-namespace-comment",
         new ClangTidyCheckFactory<NamespaceCommentCheck>());

clang-tidy/llvm/NamespaceCommentCheck.h

+//===--- NamespaceCommentCheck.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_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 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_NAMESPACE_COMMENT_CHECK_H

clang-tidy/llvm/NamespaceCommentCheck.cpp

+//===--- 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

clang-tidy/tool/ClangTidyMain.cpp

 static cl::OptionCategory ClangTidyCategory("clang-tidy options");
 
 static cl::extrahelp CommonHelp(CommonOptionsParser::HelpMessage);
 
 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"
                           "prefix. Globs are processed in order of appearance\n"
                           "in the list. Globs without '-' prefix add checks\n"
                           "with matching names to the set, globs with the '-'\n"
                           "prefix remove checks with matching names from the\n"
                           "set of enabled checks."),

unittests/clang-tidy/LLVMModuleTest.cpp

 #include "ClangTidyTest.h"
-#include "llvm/LLVMTidyModule.h"
+#include "llvm/IncludeOrderCheck.h"
+#include "llvm/NamespaceCommentCheck.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tidy {
 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
 } // namespace tidy
 } // namespace clang