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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.



================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:30
+    // Record all defined macros. We store the whole token to compare names
+    // later
+
----------------
Missing full stop.


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:32
+
+    const MacroInfo *const MI = MD->getMacroInfo();
+
----------------
You can drop the latter `const` (we don't do top-level `const` qualification with any consistency).


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:50-51
+
+    Check->addMacro(MacroNameTok.getIdentifierInfo()->getName().str(),
+                    Value.str());
+  }
----------------
You only need to add the macro name in this case, not its value, which should simplify this code considerably.


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:83
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(std::make_unique<NamespaceCommentPPCallbacks>(PP, this));
+}
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:208-215
+          std::find_if(std::begin(Macros), std::end(Macros),
+                       [&NamespaceNameInComment](const auto &Macro) {
+                         return NamespaceNameInComment == Macro.first;
+                       });
+
+      if (MacroIt != Macros.end()) {
+        return;
----------------
Rather than this, I'd suggest using `llvm::any_of()`.


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h:43
+  // 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;
----------------
Missing a full stop at the end of the sentence.


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.h:44
+  // preprocessed define
+  std::vector<std::pair<std::string, std::string>> Macros;
 };
----------------
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`.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp:19
+}
+// CHECK-FIXES: } // namespace macro_expansion
+
----------------
This seems unintuitive to me -- it should suggest the macro name instead.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/llvm-namespace-comment.cpp:27
+  void h();
+} // namespace macro_expansion
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69855/new/

https://reviews.llvm.org/D69855