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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:32
+      return;
+    if (std::string(Info->getNameStart()) != Check.MacroName)
+      return;
----------------
```
if (Info->getName() != Check.MacroName)```
Avoid constructing the string if you don't have to.


================
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))
----------------
s/classNameTok/ClassNameTok


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp:41
+      return;
+    clang::IdentifierInfo *classIdent = classNameTok->getIdentifierInfo();
+    if (!classIdent)
----------------
s/classIndent/ClassIndent


================
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) ? ";" : "");
----------------
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))```


================
Comment at: clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h:52
+
+  const std::string MacroName;
+};
----------------
I'd prefer this to be private with a getter function


================
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{{$}}
----------------
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`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80531/new/

https://reviews.llvm.org/D80531