kwk added a comment.
Thanks @njames93 and @Eugene.Zelenko for your review. Most of it, I addressed
but for some I have comments. See inline.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:36
+ // be the class name.
+ const Token *classNameTok = Args->getUnexpArgument(0);
+ if (Args->ArgNeedsPreexpansion(classNameTok, PP))
----------------
njames93 wrote:
> s/classNameTok/ClassNameTok
Ah, forgot about the CamelCase. Sorry.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:49
+ R"cpp({0}(const {0} &) = delete;
+const {0} &operator=(const {0} &) = delete{1})cpp",
+ classIdent->getNameStart(), shouldAppendSemi(Range) ? ";" : "");
----------------
njames93 wrote:
> if you wanted to, you could format this quite easily to avoid the need of
> specifying a formatter in the checks
> ```{2}const {0} &operator=(const {0} &) = delete{1})cpp",...
> Lexer::getIndentationForLine(Range.getBegin(), SM))```
Well, that does fix the indentation but for east/west-side alignment of `&`
this doesn't solve the problem, does it?
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst:31
+* Notice that the migration example above leaves the ``private`` access
+ specification untouched. This opens room for improvement, yes I know.
+
----------------
Eugene.Zelenko wrote:
> Please refer to //modernize-use-equals-delete// instead of second statement.
`modernize-use-equals-delete` doesn't help because it also does not remove the
`private` access. Why refer to it?
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp:36-38
+// CHECK-MESSAGES-NEXT-DEFAULT: {{^}}
DISALLOW_COPY_AND_ASSIGN(TestClass1);{{$}}
+// CHECK-MESSAGES-NEXT-DEFAULT: {{^}} ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~{{$}}
+// CHECK-MESSAGES-NEXT-DEFAULT: {{^}} TestClass1(const TestClass1 &) =
delete;const TestClass1 &operator=(const TestClass1 &) = delete{{$}}
----------------
njames93 wrote:
> These really aren't important to test for, so long as the diagnostic and fix
> it are checked, that's all you need to worry about. Like how you have it for
> `TestClass2`
Sure, I get that. I think I left them in because I wasn't that familiar with
how `%check_clang_tidy` calls `FileCheck` two times. Thanks for catching this.
Removed.
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
