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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
