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

[Nathan James via Phabricator via cfe-commits]

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