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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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));
+}
----------------
twardakm wrote:
> aaron.ballman wrote:
> > 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.
> 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?
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.


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:101-104
+    if (!IsNamespaceMacroExpansion)
+      Fix.append(" ").append(ND->getNameAsString());
+    else
+      Fix.append(" ").append(MacroDefinition);
----------------
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.)


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:135
+    const StringRef NameSpaceName) {
+  const auto &MacroIt = std::find_if(std::begin(Macros), std::end(Macros),
+                                     [&NameSpaceName](const auto &Macro) {
----------------
`llvm::find_if(Macros, ...);`


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:247
+      // Allow using macro definitions in closing comment.
+      if (isNamespaceMacroDefinition(NamespaceNameInComment)) {
+        return;
----------------
Can elide braces.


================
Comment at: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp:287
+
+  if (IsNamespaceMacroExpansion) {
+    NestedNamespaceName = MacroDefinition;
----------------
Elide braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69855