njames93 added a comment.
In D80531#2053969 <https://reviews.llvm.org/D80531#2053969>, @kwk wrote:
> @Eugene.Zelenko thank you for the review. I've fixed all places that you've
> mentioned. And have a question about one thing in particular. See inline.
>
> Do you by any chance know why `llvm-lit` keeps complaining when I use
> `[[@LINE-1]]` in my tests as so many other tests do?
It's complaining because your check lines are 2 lines after the diagnostic so
you should use `[[@LINE-2]]`
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:28
+ SourceRange Range, const MacroArgs *Args) override {
+ IdentifierInfo *identifierInfo = MacroNameTok.getIdentifierInfo();
+ if (!identifierInfo)
----------------
clang-tidy fail on the variables, please rename them to use `CamelCase` format.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:51
+ // pre-written code snippet. But for now, this is okay.
+ std::string Replacement =
+ className + "(const " + className + " &) = delete;";
----------------
nit:
```
std::string Replacement = llvm::formatv(
R"cpp({0}(const {0} &) = delete;
const {0} &operator=(const {0} &) = delete{1})cpp",
classIdent->getNameStart(), FinalizeWithSemicolon ? ";" : "");```
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:66
+
+ ClangTidyCheck &Check;
+ Preprocessor &PP;
----------------
nit: Shouldn't this be a reference to a
`ReplaceDisallowCopyAndAssignMacroCheck`? Doing so will allow you to use a
getter in the Check for the `MacroName` option to save storing a copy of that
as well inside the callback.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h:52-53
+
+ std::string MacroName;
+ bool FinalizeWithSemicolon;
+};
----------------
These can both be marked `const`
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:41
+
+.. option:: FinalizeWithSemicolon
+
----------------
This option should be removed and instead infer the value dynamically by
checking the token directly after the macro use to see if its a semi-colon.
Something like this in the PPCallback should get you there
```
bool shouldAppendSemi(SourceRange MacroLoc) {
llvm::Optional<Token> Next =
Lexer::findNextToken(MacroLoc.getEnd(), SM, PP.getLangOpts());
return !(Next && Next->is(tok::semi));
}```
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
