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

[Nathan James via Phabricator via cfe-commits]

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