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