[clang-tidy] Fix llvm-namespace-comment for macro expansions

[twardakm]

Summary

If a namespace is a macro name, it should be allowed to close the
namespace with the same name.

This commit adds also unit tests for llvm-namespace-comment.

https://bugs.llvm.org/show_bug.cgi?id=26274

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

253

Is this empty line necessary?

[twardakm]

Remove unnecessary line

[aaron.ballman]

There's a design question about why it should accept the expanded macro name instead of requiring the user to write the macro. I am worried about allowing the expanded form because the presumable use of a macro is to control the name, so this invites name mismatches in other configuration modes.

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

30

Missing full stop.

32

You can drop the latter const (we don't do top-level const qualification with any consistency).

50–51

You only need to add the macro name in this case, not its value, which should simplify this code considerably.

83

Rather than making an entire new object for PP callbacks, why not make NamespaceCommentCheck inherit from PPCallbacks? It seems like it would simplify the interface somewhat.

245–252

Rather than this, I'd suggest using llvm::any_of().

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h

50

Missing a full stop at the end of the sentence.

51

I think a better approach is to use a set of some sort because the value of the macro is never used in the check. Probably a SmallPtrSet over StringRef.

clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp

20

This seems unintuitive to me -- it should suggest the macro name instead.

28

This also seems unintuitive to me, I would have wanted this to diagnose and provide a fixit for suggesting the macro name, not its expansion.

[twardakm]

Apply Aaron's comments

• Fix missing namespace to macro definition instead of extension
• Print macro definition also in warning message

[twardakm]

Thanks for the review!

@aaron.ballman take a look at new revision and let me know if something needs to be fixed. Thanks!

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

50–51

See comment above, now both value and definition is used

83

I think the check hast to inherit from ClangTidyCheck?

I duplicated the solution from other checks (e.g. IdentifierNamingCheck.cpp). Could you please point to some existing check which implements your idea?

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h

51

After applying other comments, both value and definition is used, therefore I stayed with pair of strings. Let me know if you think this comment is still valid

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

83

I don't know if we have other checks doing this -- I was thinking of using multiple inheritance in this case, because the callbacks are effectively a mixin.

That said, I don't insist on this change.

101–104

Would this work instead Fix.append(" ").append(IsNamespaceMacroExpansion ? MacroDefinition : ND->getName());?

(You may have to check the behavioral difference of getName() vs getNameAsString() to be sure.)

133

llvm::find_if(Macros, ...);

245

Can elide braces.

284

Elide braces

[twardakm]

Fix style issues

[twardakm]

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

83

The problem which I see is that addPPCallbacks takes ownership of the object passed to it. I couldn't find any other way of invoking PP callbacks, so I'll leave it as is.

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

83

Ah, that makes sense. Thank you for investigating it. :-)

[twardakm]

@aaron.ballman thanks for the review :) Can you please push the change on my behalf? I don't have commit rights.

[aaron.ballman]

In D69855#1754047, @twardakm wrote:

@aaron.ballman thanks for the review :) Can you please push the change on my behalf? I don't have commit rights.

I've commit on your behalf in 4736d63f752f8d13f4c6a9afd558565c32119718, thank you for the patch!

[alexfh]

There's a problem with this patch. Consider the following code:

// In some remote header:
#define SOME_RANDOM_MACRO internal

// In a completely unrelated file that transitively includes the header:
namespace internal {
void f();
} // namespace internal

It makes the check think that every namespace named internal has something to do with SOME_RANDOM_MACRO.

$ clang_tidy -checks=-*,llvm-* /tmp/q.cc --
1 warning generated.
/tmp/q.cc:7:2: warning: namespace 'SOME_RANDOM_MACRO' ends with a comment that refers to an expansion of macro [llvm-namespace-comment]
} // namespace internal
 ^~~~~~~~~~~~~~~~~~~~~~
  // namespace SOME_RANDOM_MACRO
/tmp/q.cc:5:11: note: namespace 'SOME_RANDOM_MACRO' starts here
namespace internal {
          ^

This is definitely wrong and it introduces tons of false positives in our code. It seems to me that the check shouldn't look any further than what is actually spelled in the namespace declaration. It shouldn't try to match macros to their expansions or vice versa. I'll see whether I can implement this quickly, otherwise I'll just revert this patch to unbreak the checker.

[gribozavr2]

@twardakm: I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias?

[twardakm]

The reason for this whole patch was a bug in clang-tidy, which caused warning in the following code:

#define SOME_RANDOM_MACRO macro

namespace SOME_RANDOM_MACRO {

} // namespace SOME_RANDOM_MACRO

and clang-tidy suggested:

namespace SOME_RANDOM_MACRO {

} // namespace macro

which is obviously wrong.

During review we decided that it would be a good idea (apparently not so good) to detect macro expansions and force users to use only macro definitions. This could be useful to avoid mixing macro definitions with macro expansions.

@alexfh, @gribozavr2, @aaron.ballman I think the best way out here is just to implement the basic fix for the above problem and only allow to use macro definition in closing comment and skip checking macro expansions completely. If you agree I will provide a patch for that.

[gribozavr2]

In D69855#1769318, @twardakm wrote:

@alexfh, @gribozavr2, @aaron.ballman I think the best way out here is just to implement the basic fix for the above problem and only allow to use macro definition in closing comment and skip checking macro expansions completely. If you agree I will provide a patch for that.

alexfh already provided a fix, please review and ensure that it fixes your case: https://reviews.llvm.org/D70974

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h

 /// http://llvm.org/docs/CodingStandards.html#namespace-indentation
 ///
 /// https://google.github.io/styleguide/cppguide.html#Namespaces
 class NamespaceCommentCheck : public ClangTidyCheck {
 public:
   NamespaceCommentCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+
+  void addMacro(const std::string &Name, const std::string &Value) noexcept;
 
 private:
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
+  std::string getNamespaceComment(const NamespaceDecl *ND,
+                                  bool InsertLineBreak);
+  std::string getNamespaceComment(const std::string &NameSpaceName,
+                                  bool InsertLineBreak);
+  bool isNamespaceMacroDefinition(const StringRef NameSpaceName);
+  std::tuple<bool, StringRef>
+  isNamespaceMacroExpansion(const StringRef NameSpaceName);
 
   llvm::Regex NamespaceCommentPattern;
   const unsigned ShortNamespaceLines;
   const unsigned SpacesBeforeComments;
   llvm::SmallVector<SourceLocation, 4> Ends;
+
+  // Store macros to verify that warning is not thrown when namespace name is a
+  // preprocessed define.
+  std::vector<std::pair<std::string, std::string>> Macros;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NAMESPACECOMMENTCHECK_H

clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp

 #include "llvm/ADT/StringExtras.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
+namespace {
+class NamespaceCommentPPCallbacks : public PPCallbacks {
+public:
+  NamespaceCommentPPCallbacks(Preprocessor *PP, NamespaceCommentCheck *Check)
+      : PP(PP), Check(Check) {}
+
+  void MacroDefined(const Token &MacroNameTok, const MacroDirective *MD) {
+    // Record all defined macros. We store the whole token to compare names
+    // later.
+
+    const MacroInfo * MI = MD->getMacroInfo();
+
+    if (MI->isFunctionLike())
+      return;
+
+    std::string ValueBuffer;
+    llvm::raw_string_ostream Value(ValueBuffer);
+
+    SmallString<128> SpellingBuffer;
+    bool First = true;
+    for (const auto &T : MI->tokens()) {
+      if (!First && T.hasLeadingSpace())
+        Value << ' ';
+
+      Value << PP->getSpelling(T, SpellingBuffer);
+      First = false;
+    }
+
+    Check->addMacro(MacroNameTok.getIdentifierInfo()->getName().str(),
+                    Value.str());
+  }
+
+private:
+  Preprocessor *PP;
+  NamespaceCommentCheck *Check;
+};
+} // namespace
+
 NamespaceCommentCheck::NamespaceCommentCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       NamespaceCommentPattern("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
                               "namespace( +([a-zA-Z0-9_:]+))?\\.? *(\\*/)?$",
                               llvm::Regex::IgnoreCase),
       ShortNamespaceLines(Options.get("ShortNamespaceLines", 1u)),
       SpacesBeforeComments(Options.get("SpacesBeforeComments", 1u)) {}
 
 void NamespaceCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "ShortNamespaceLines", ShortNamespaceLines);
   Options.store(Opts, "SpacesBeforeComments", SpacesBeforeComments);
 }
 
 void NamespaceCommentCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (getLangOpts().CPlusPlus)
     Finder->addMatcher(namespaceDecl().bind("namespace"), this);
 }
 
+void NamespaceCommentCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(std::make_unique<NamespaceCommentPPCallbacks>(PP, this));
+}
+
 static bool locationsInSameFile(const SourceManager &Sources,
                                 SourceLocation Loc1, SourceLocation Loc2) {
   return Loc1.isFileID() && Loc2.isFileID() &&
          Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
 }
 
-static std::string getNamespaceComment(const NamespaceDecl *ND,
+std::string NamespaceCommentCheck::getNamespaceComment(const NamespaceDecl *ND,
                                                        bool InsertLineBreak) {
   std::string Fix = "// namespace";
-  if (!ND->isAnonymousNamespace())
-    Fix.append(" ").append(ND->getNameAsString());
+  if (!ND->isAnonymousNamespace()) {
+    bool IsNamespaceMacroExpansion;
+    StringRef MacroDefinition;
+    std::tie(IsNamespaceMacroExpansion, MacroDefinition) =
+        isNamespaceMacroExpansion(ND->getName());
+
+    Fix.append(" ").append(IsNamespaceMacroExpansion ? MacroDefinition
+                                                     : ND->getName());
+  }
   if (InsertLineBreak)
     Fix.append("\n");
   return Fix;
 }
 
-static std::string getNamespaceComment(const std::string &NameSpaceName,
+std::string
+NamespaceCommentCheck::getNamespaceComment(const std::string &NameSpaceName,
                                            bool InsertLineBreak) {
   std::string Fix = "// namespace ";
   Fix.append(NameSpaceName);
   if (InsertLineBreak)
     Fix.append("\n");
   return Fix;
 }
 
+void NamespaceCommentCheck::addMacro(const std::string &Name,
+                                     const std::string &Value) noexcept {
+  Macros.emplace_back(Name, Value);
+}
+
+bool NamespaceCommentCheck::isNamespaceMacroDefinition(
+    const StringRef NameSpaceName) {
+  return llvm::any_of(Macros, [&NameSpaceName](const auto &Macro) {
+    return NameSpaceName == Macro.first;
+  });
+}
+
+std::tuple<bool, StringRef> NamespaceCommentCheck::isNamespaceMacroExpansion(
+    const StringRef NameSpaceName) {
+  const auto &MacroIt =
+      llvm::find_if(Macros, [&NameSpaceName](const auto &Macro) {
+        return NameSpaceName == Macro.second;
+      });
+
+  const bool IsNamespaceMacroExpansion = Macros.end() != MacroIt;
+
+  return std::make_tuple(IsNamespaceMacroExpansion,
+                         IsNamespaceMacroExpansion ? StringRef(MacroIt->first)
+                                                   : NameSpaceName);
+}
+
 void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
   const SourceManager &Sources = *Result.SourceManager;
 
   if (!locationsInSameFile(Sources, ND->getBeginLoc(), ND->getRBraceLoc()))
     return;
 
   // Don't require closing comments for namespaces spanning less than certain
void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
   // 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, 7> Groups;
     if (NamespaceCommentPattern.match(Comment, &Groups)) {
       StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] : "";
       StringRef Anonymous = Groups.size() > 3 ? Groups[3] : "";
 
+      // Don't allow to use macro expansion in closing comment.
+      // FIXME: Use Structured Bindings once C++17 features will be enabled.
+      bool IsNamespaceMacroExpansion;
+      StringRef MacroDefinition;
+      std::tie(IsNamespaceMacroExpansion, MacroDefinition) =
+          isNamespaceMacroExpansion(NamespaceNameInComment);
+
       if (IsNested && NestedNamespaceName == NamespaceNameInComment) {
         // C++17 nested namespace.
         return;
       } else if ((ND->isAnonymousNamespace() &&
                   NamespaceNameInComment.empty()) ||
-                 (ND->getNameAsString() == NamespaceNameInComment &&
-                  Anonymous.empty())) {
+                 (((ND->getNameAsString() == NamespaceNameInComment) &&
+                   Anonymous.empty()) &&
+                  !IsNamespaceMacroExpansion)) {
         // Check if the namespace in the comment is the same.
         // FIXME: Maybe we need a strict mode, where we always fix namespace
         // comments with different format.
         return;
       }
 
+      // Allow using macro definitions in closing comment.
+      if (isNamespaceMacroDefinition(NamespaceNameInComment))
+        return;
+
       // Otherwise we need to fix the comment.
       NeedLineBreak = Comment.startswith("/*");
       OldCommentRange =
           SourceRange(AfterRBrace, Loc.getLocWithOffset(Tok.getLength()));
-      Message =
-          (llvm::Twine(
-               "%0 ends with a comment that refers to a wrong namespace '") +
+
+      if (IsNamespaceMacroExpansion) {
+        Message = (llvm::Twine("%0 ends with a comment that refers to an "
+                               "expansion of macro"))
+                      .str();
+        NestedNamespaceName = MacroDefinition;
+      } else {
+        Message = (llvm::Twine("%0 ends with a comment that refers to a "
+                               "wrong namespace '") +
                    NamespaceNameInComment + "'")
                       .str();
+      }
+
     } else if (Comment.startswith("//")) {
       // Assume that this is an unrecognized form of a namespace closing line
       // comment. Replace it.
       NeedLineBreak = false;
       OldCommentRange =
           SourceRange(AfterRBrace, Loc.getLocWithOffset(Tok.getLength()));
       Message = "%0 ends with an unrecognized comment";
     }
     // If it's a block comment, just move it to the next line, as it can be
     // multi-line or there may be other tokens behind it.
   }
 
+  // Print Macro definition instead of expansion.
+  // FIXME: Use Structured Bindings once C++17 features will be enabled.
+  bool IsNamespaceMacroExpansion;
+  StringRef MacroDefinition;
+  std::tie(IsNamespaceMacroExpansion, MacroDefinition) =
+      isNamespaceMacroExpansion(NestedNamespaceName);
+
+  if (IsNamespaceMacroExpansion)
+    NestedNamespaceName = MacroDefinition;
+
   std::string NamespaceName =
       ND->isAnonymousNamespace()
           ? "anonymous namespace"
           : ("namespace '" + NestedNamespaceName.str() + "'");
 
   // Place diagnostic at an old comment, or closing brace if we did not have it.
   SourceLocation DiagLoc =
       OldCommentRange.getBegin() != OldCommentRange.getEnd()

clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp

 void f(); // So that the namespace isn't empty.
 // 1
 // 2
 // 3
 // 4
 // 5
 // 6
 // 7
-// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'macro_expansion' not terminated with
-// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'MACRO' not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'MACRO' starts here
 }
-// CHECK-FIXES: }  // namespace macro_expansion
+// CHECK-FIXES: }  // namespace MACRO
 
 namespace short1 {
 namespace short2 {
 // Namespaces covering 10 lines or fewer are exempt from this rule.
 
 
 
 

clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp

+// RUN: %check_clang_tidy %s llvm-namespace-comment %t
+
+namespace n1 {
+namespace n2 {
+  void f();
+
+
+  // CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment]
+}}
+// CHECK-FIXES: } // namespace n2
+// CHECK-FIXES: } // namespace n1
+
+#define MACRO macro_expansion
+namespace MACRO {
+  void f();
+  // CHECK-MESSAGES: :[[@LINE+1]]:1: warning: namespace 'MACRO' not terminated with a closing comment [llvm-namespace-comment]
+}
+// CHECK-FIXES: } // namespace MACRO
+
+namespace MACRO {
+  void g();
+} // namespace MACRO
+
+namespace MACRO {
+  void h();
+  // CHECK-MESSAGES: :[[@LINE+1]]:2: warning: namespace 'MACRO' ends with a comment that refers to an expansion of macro [llvm-namespace-comment]
+} // namespace macro_expansion
+// CHECK-FIXES: } // namespace MACRO
+
+namespace n1 {
+namespace MACRO {
+namespace n2 {
+  void f();
+  // CHECK-MESSAGES: :[[@LINE+3]]:1: warning: namespace 'n2' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'MACRO' not terminated with a closing comment [llvm-namespace-comment]
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: namespace 'n1' not terminated with a closing comment [llvm-namespace-comment]
+}}}
+// CHECK-FIXES: } // namespace n2
+// CHECK-FIXES: } // namespace MACRO
+// CHECK-FIXES: } // namespace n1