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

[Marcin Twardak via Phabricator via cfe-commits]

twardakm marked 9 inline comments as done.
twardakm added a comment.

Thanks for the review!

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



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


================
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));
+}
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69855