[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

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