njames93 added a comment.
Few changes and nits then LGTM
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:25
+ const SourceManager &SM)
+ : Check(Check), PP(PP), SM(SM) {}
+
----------------
nit: You don't need to store a reference to the `SourceManager` as the
`Preprocessor` holds a reference to it.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:32
+ return;
+ if (Info->getNameStart() != Check.MacroName)
+ return;
----------------
Use `Info->getName()` here, the Length is stored internally so its not
expensive to get and makes the code look more readable.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:45-46
+
+ // FIXME: Maybe someday I will know how to expand the macro and not use my
+ // pre-written code snippet. But for now, this is okay.
+ std::string Replacement = llvm::formatv(
----------------
This FIXME can probably be removed as expanding the macro isn't a good idea.
The macro should expand to just defining the function rather than deleting it
like we want.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:50
+const {0} &operator=(const {0} &) = delete{1})cpp",
+ ClassIdent->getNameStart(), shouldAppendSemi(Range) ? ";" : "");
+
----------------
ditto: use `ClassIdent->getName()` as well.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:52
+
+ Check.diag(MacroNameTok.getLocation(), "using copy and assign macro '%0'")
+ << Check.MacroName
----------------
nit: Not a fan of the warning message, would prefer something like
`prefer deleting copy constructor and assignment operator over using macro
'%0'` to explain what we are trying to do. Though if someone can think of
something shorter that gets the same point I'm open to that.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:59
+private:
+ /// \returns \c true if the next token after the given \a MacroLoc is \b not
a
+ /// semicolon.
----------------
Use `\p` before `MacroLoc` instead of `\a` as its a parameter.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h:52
+
+ const std::string MacroName;
+};
----------------
Make this private with a get function and I'll be happy.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80531/new/
https://reviews.llvm.org/D80531
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
