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

[kwk]

Summary

This check finds macro expansions of DISALLOW_COPY_AND_ASSIGN(Type) and
replaces them with a deleted copy constructor and a deleted assignment operator.

Before the delete keyword was introduced in C++11 it was common practice to
declare a copy constructor and an assignment operator as a private members. This
effectively makes them unusable to the public API of a class.

With the advent of the delete keyword in C++11 we can abandon the
private access of the copy constructor and the assignment operator and
delete the methods entirely.

Migration example:

class Foo {
  private:
  -  DISALLOW_COPY_AND_ASSIGN(Foo);
  +  Foo(const Foo &) = delete;
  +  const Foo &operator=(const Foo &) = delete;
  };

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h

47

I think check should work in C++11 or newer.

clang-tools-extra/docs/clang-tidy/checks/list.rst

192

I think this and some other changes are unrelated.

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

7

Just Finds.

32

modernize-use-equals-delete takes care about this.

34

Please move this to comments into code.

37

Please use single back-ticks for options.

45

Please use single back-ticks for option values.

49

Please use single back-ticks for option values.

[kwk]

• Make ReplaceDisallowCopyAndAssignMacroCheck only work in C++11 and above
• Remove unrelated changes from clang-tools-extra/docs/clang-tidy/checks/list.rst
• documentation simplification
• Move comment into code
• use single ticks

[kwk]

@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?

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

32

Yes modernize-use-equals-delete does, this but it only notifies about it. Do you want me to notify with "deleted member function should be public" as well? Shouldn't I maybe just recommend to run the modernize-use-equals-delete after running my check?

[kwk]

• Move comment about FinalizeWithSemicolon into code

[njames93]

In 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]]

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp

29

clang-tidy fail on the variables, please rename them to use CamelCase format.

52

nit:

std::string Replacement = llvm::formatv(
    R"cpp({0}(const {0} &) = delete;
const {0} &operator=(const {0} &) = delete{1})cpp",
    classIdent->getNameStart(), FinalizeWithSemicolon ? ";" : "");

67

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.

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h

53–54

These can both be marked const

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

42

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));
}

[kwk]

Thank you @njames93 for the review. I'm implementing what you wrote.

[kwk]

• Wrap RUN lines
• Use CamelCase variable name
• Simplify access to options
• Make check options const
• Use two-line placeholder
• Automate placement of semicolon at the end

[kwk]

@njames93 I've implemented everything you've mentioned. This simplified a lot.

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

42

Thank you so much for this!

[njames93]

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp

33

if (Info->getName() != Check.MacroName)

Avoid constructing the string if you don't have to.

37

s/classNameTok/ClassNameTok

42

s/classIndent/ClassIndent

50

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))

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h

53

I'd prefer this to be private with a getter function

clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp

37–39

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

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

24

I think two code snippets would be more readable.

30

Please insert empty line below.

32

Please refer to modernize-use-equals-delete instead of second statement.

[kwk]

• Avoid constructing string
• CamelCase for vars
• more readable snippets for documentation
• Added empty line after header in documentation
• Remove sentence from doc
• Remove unimportant tests

[kwk]

Thanks @njames93 and @Eugene.Zelenko for your review. Most of it, I addressed but for some I have comments. See inline.

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp

37

Ah, forgot about the CamelCase. Sorry.

50

Well, that does fix the indentation but for east/west-side alignment of & this doesn't solve the problem, does it?

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

32

modernize-use-equals-delete doesn't help because it also does not remove the private access. Why refer to it?

clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp

37–39

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.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

32

It complaines about deleted stuff in private section.

[kwk]

• Refer to modernize-use-equals-delete for warning about deleted functions in private sections

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

32

Agreed, that makes sense. Sorry for not getting that right away. Afterall I did write about that above :)

Yes modernize-use-equals-delete does, this but it only notifies about it.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

42

See Release Notes on how to create links on documentation.

[kwk]

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

42

I did look at another test (https://raw.githubusercontent.com/llvm/llvm-project/master/clang-tools-extra/docs/clang-tidy/checks/readability-convert-member-functions-to-static.rst) and copied from there. But I will now change it.

[kwk]

• Adjust link to documentation

[kwk]

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

42

@Eugene.Zelenko Done.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

42

clang-tidy/checks/ is not needed. All checks documentation is in same directory.

[kwk]

• fix link in doc

[kwk]

@Eugene.Zelenko thank you for your patience with me. I fixed the link.

[kwk]

@njames93 and @Eugene.Zelenko do you guys think this patch is ready to be approved and land?

[njames93]

Few changes and nits then LGTM

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp

26

nit: You don't need to store a reference to the SourceManager as the Preprocessor holds a reference to it.

33

Use Info->getName() here, the Length is stored internally so its not expensive to get and makes the code look more readable.

46–47

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.

51

ditto: use ClassIdent->getName() as well.

53

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.

60

Use \p before MacroLoc instead of \a as its a parameter.

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h

53

Make this private with a get function and I'll be happy.

[kwk]

• Don't store SourceManager but get it from Preprocessor
• Use getName instead of getNameStart
• Remove FIXME
• change warning message
• doxygen: \a -> \p
• Make MacroName private and add getter

[kwk]

@njames93 I've addressed all your comments and hope the patch is good to land now :)

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp

26

@njames93 thank you for letting me know. I passed it along because the ´ClangTidyCheck::registerPPCallbacks` that I override actually accepts both, SourceManager and the PP:

virtual void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                                   Preprocessor *ModuleExpanderPP) {}

Other checks like MakeSmartPtrCheck, PassByValueCheck also do this similar to mine.

Anyway, I've changed the code to your liking.

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h

53

Done. But since the variable is const having it public shouldn't allow a caller do anything but read the macro name. But I understand that convention is king ;)

[kwk]

Marked more comments as "done" after going through the code again.

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp

50

Well, yes but it will only work for the indentation not for anything else, like east/west alignment of & for example.

[njames93]

LGTM with 2 small nits, but I'd still give a few days see if anyone else wants to weight in.

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h

53

Return by reference here.

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

18

Put a colon on the end of this line.

27

Ditto here.

[kwk]

• Return macro name by reference
• Add colon to paragraph showing code

[kwk]

Done.

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h

53

Yeah, that makes sense. Stupid me.

[kwk]

In D80531#2069383, @njames93 wrote:

LGTM with 2 small nits, but I'd still give a few days see if anyone else wants to weight in.

I'm okay with this but I'd like to understand when it's time to wait for others? Only when a patch is approved or from the beginning of patch's life? I mean who looks at patches that are approved unless you filter for the amount of approvers? How many approvers must there be?

[dblaikie]

In D80531#2069637, @kwk wrote:

In D80531#2069383, @njames93 wrote:

LGTM with 2 small nits, but I'd still give a few days see if anyone else wants to weight in.

I'm okay with this but I'd like to understand when it's time to wait for others? Only when a patch is approved or from the beginning of patch's life? I mean who looks at patches that are approved unless you filter for the amount of approvers? How many approvers must there be?

FWIW, I tend to - I have a queue of things to review, which doesn't have /much/ to do with whether they've already been reviewed. Sometimes, sometimes not - usually if I've decided it's worth looking at I'll still at least glance over it (enough to see if someone's asking for further consensus, etc).

While this is rather belated feedback, I've been seeing failures of this test consistently since it was committed, I think. Maybe there's something corrupted in my local client, but figured I'd mention/ask about it, in case it's useful. (+ @sammccall too):

FAIL: Clang Tools :: clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp (18811 of 74169)
******************** TEST 'Clang Tools :: clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /usr/bin/python3.8 /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py -format-style=LLVM -check-suffix=DEFAULT /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp    modernize-replace-disallow-copy-and-assign-macro /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp
: 'RUN: at line 4';   /usr/bin/python3.8 /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py -format-style=LLVM -check-suffix=DIFFERENT-NAME /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp   modernize-replace-disallow-copy-and-assign-macro /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp   -config="{CheckOptions: [    {key: modernize-replace-disallow-copy-and-assign-macro.MacroName,     value: MY_MACRO_NAME}]}"
: 'RUN: at line 10';   /usr/bin/python3.8 /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py -format-style=LLVM -check-suffix=FINALIZE /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp   modernize-replace-disallow-copy-and-assign-macro /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp   -config="{CheckOptions: [    {key: modernize-replace-disallow-copy-and-assign-macro.MacroName,     value: DISALLOW_COPY_AND_ASSIGN_FINALIZE}]}"
: 'RUN: at line 16';   clang-tidy /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp -checks="-*,modernize-replace-disallow-copy-and-assign-macro"    -config="{CheckOptions: [     {key: modernize-replace-disallow-copy-and-assign-macro.MacroName,      value: DISALLOW_COPY_AND_ASSIGN_MORE_AGUMENTS}]}" | count 0
: 'RUN: at line 21';   clang-tidy /usr/local/google/home/blaikie/dev/llvm/src/clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp -checks="-*,modernize-replace-disallow-copy-and-assign-macro"    -config="{CheckOptions: [     {key: modernize-replace-disallow-copy-and-assign-macro.MacroName,      value: DISALLOW_COPY_AND_ASSIGN_NEEDS_PREEXPANSION}]}" | count 0
--
Exit Code: 1

Command Output (stdout):
--
Running ['clang-tidy', '/usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp', '-fix', '--checks=-*,modernize-replace-disallow-copy-and-assign-macro', '-format-style=LLVM', '--', '-std=c++11', '-nostdinc++']...
------------------------ clang-tidy output -----------------------
1 warning generated.
/usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp:33:3: warning: prefer deleting copy constructor and assignment operator over using macro 'DISALLOW_COPY_AND_ASSIGN' [modernize-replace-disallow-copy-and-assign-macro]
  DISALLOW_COPY_AND_ASSIGN(TestClass1);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp:33:3: note: FIX-IT applied suggested code changes
clang-tidy applied 1 of 1 suggested fixes.

------------------------------------------------------------------
------------------------------ Fixes -----------------------------
--- /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp.orig     2020-09-09 16:32:20.416366536 -0700
+++ /usr/local/google/home/blaikie/dev/llvm/build/default/tools/clang/tools/extra/test/clang-tidy/checkers/Output/modernize-replace-disallow-copy-and-assign-macro.cpp.tmp.cpp  2020-09-09 16:32:20.476366112 -0700
@@ -30,7 +30,8 @@

 class TestClass1 {
 private:
-  DISALLOW_COPY_AND_ASSIGN(TestClass1);
+  TestClass1(const TestClass1 &) = delete;
+  const TestClass1 &operator=(const TestClass1 &) = delete;
 };
 //
 //

<lots more similar failures>

[kkleine]

@dblaikie sorry for not getting to it for so long. I'm taking a look at the problem you've described now.

[kkleine]

Hi @dblaikie . I did run ninja check-all and /bin/llvm-lit -av ../clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp on this very new revision (abd70fb3983f342bc1c90f9c70a7b59790ad5206) manually but I don't see this error coming up. Can you please try to test if you see the error on a fresh build?

[dblaikie]

In D80531#2284388, @kkleine wrote:

Hi @dblaikie . I did run ninja check-all and /bin/llvm-lit -av ../clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp on this very new revision (abd70fb3983f342bc1c90f9c70a7b59790ad5206) manually but I don't see this error coming up. Can you please try to test if you see the error on a fresh build?

Hey - thanks for gettiing back to me.

I've tried a /bunch/ of variants of my usual build lately, and this seems to be what I've hit a fairly reliable repro/no-repro:

$ CC=clang CXX=clang++ cmake -G Ninja -DLLVM_ENABLE_WERROR=true  -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra' path/to/llvm/src
$ ninja check-all

If I turn of LLVM_ENABLE_WERROR (either by deleting the whole build tree and starting again, omitting the -DLLVM_ENABLE_WERROR=true part, or by manually editing the existing CMakeCache.txt file) the failure on this test case goes away. If I turn on Werror again, the failure turns up again.

It's possible (I guess likely, or this would be failing on a bunch of buildbots, I imagine) that this has somtehing to do with my specific local build of clang that I'm using to build - but even then, it seems problematic that the LLVM_ENABLE_WERROR is affecting this test execution in any way.

Also, just in general - I wonder if this test and test infrastructure (check_clang_tidy) could be made more legible in some way (at least the failure message/reporting) - perhaps it's just my lack of familiarity and anyone working on the tool would be able to read what was going wrong here, but I can't make heads or tails of the failure, couldn't figure out how to extract a file showing the good/bad behavior so I Could compare it, etc.

[kkleine]

@dblaikie sorry for the late feedback. The LLVM_ENABLE_WERROR:BOOL will "Stop and fail the build, if a compiler warning is triggered. Defaults to OFF." I wonder if any other test fails from clang tidy because. My test explicitly checks that a warning is issued (e.g. // CHECK-MESSAGES-DEFAULT: :[[@LINE-2]]:3: warning: prefer deleting) and when the LLVM_ENABLE_WERROR propagates to this piece, it will indeed fail the build. But I wonder why this doesn't happen to the other clang-tidy checks. @njames93 @Eugene.Zelenko any ideas?

[njames93]

In D80531#2352944, @kkleine wrote:

@dblaikie sorry for the late feedback. The LLVM_ENABLE_WERROR:BOOL will "Stop and fail the build, if a compiler warning is triggered. Defaults to OFF." I wonder if any other test fails from clang tidy because. My test explicitly checks that a warning is issued (e.g. // CHECK-MESSAGES-DEFAULT: :[[@LINE-2]]:3: warning: prefer deleting) and when the LLVM_ENABLE_WERROR propagates to this piece, it will indeed fail the build. But I wonder why this doesn't happen to the other clang-tidy checks. @njames93 @Eugene.Zelenko any ideas?

The reason WERROR causes this to fail is the tests are generating a warning Wextra-semi for the ; that appear after each macro usage in the class. clang-tidy would ignore this diagnostic as its set-up to ignore all warnings from clang.
However when Werror is set, that warning is promoted to an error, and clang-tidy will emit all errors, regardless of their origin.
This causes there to be extra diagnostics in the tests, though only in the tests where you invoke clang-tidy directly. Not sure why that obeys LLVM_ENABLE_WERROR likewise not sure why check_clang_tidy doesn't obey it.
Anyway the simple fix is to just remove the ; after the macro usages in the class

[kkleine]

@njames93 I could do that but the original Macros had were defined without a semicolon at the end and one had to add it manually. See this revision in which I replaced some occurrences of DISALLOW_COPY_AND_ASSIGN: eaebcbc67926a18befaa297f1778edde63baec9b. What do you suggest? Keep the semicolon to more closely match the original macros or remove it to make the test happy?

[njames93]

In D80531#2353314, @kkleine wrote:

@njames93 I could do that but the original Macros had were defined without a semicolon at the end and one had to add it manually. See this revision in which I replaced some occurrences of DISALLOW_COPY_AND_ASSIGN: eaebcbc67926a18befaa297f1778edde63baec9b. What do you suggest? Keep the semicolon to more closely match the original macros or remove it to make the test happy?

Or add -Wno-extra-semi in the tests where you invoke clang-tidy directly, That'll silence the warnings and leave the rest of the test as-is.

[dblaikie]

In D80531#2353268, @njames93 wrote:

In D80531#2352944, @kkleine wrote:

@dblaikie sorry for the late feedback. The LLVM_ENABLE_WERROR:BOOL will "Stop and fail the build, if a compiler warning is triggered. Defaults to OFF." I wonder if any other test fails from clang tidy because. My test explicitly checks that a warning is issued (e.g. // CHECK-MESSAGES-DEFAULT: :[[@LINE-2]]:3: warning: prefer deleting) and when the LLVM_ENABLE_WERROR propagates to this piece, it will indeed fail the build. But I wonder why this doesn't happen to the other clang-tidy checks. @njames93 @Eugene.Zelenko any ideas?

The reason WERROR causes this to fail is the tests are generating a warning Wextra-semi for the ; that appear after each macro usage in the class. clang-tidy would ignore this diagnostic as its set-up to ignore all warnings from clang.
However when Werror is set, that warning is promoted to an error, and clang-tidy will emit all errors, regardless of their origin.
This causes there to be extra diagnostics in the tests, though only in the tests where you invoke clang-tidy directly. Not sure why that obeys LLVM_ENABLE_WERROR likewise not sure why check_clang_tidy doesn't obey it.
Anyway the simple fix is to just remove the ; after the macro usages in the class

Oh, neat - thanks for identifying that! Probably be good for someone to figure out how to make the tests independent of the LLVM build flags like LLVM_ENABLE_WERROR in general.

In D80531#2353314, @kkleine wrote:

@njames93 I could do that but the original Macros had were defined without a semicolon at the end and one had to add it manually. See this revision in which I replaced some occurrences of DISALLOW_COPY_AND_ASSIGN: eaebcbc67926a18befaa297f1778edde63baec9b. What do you suggest? Keep the semicolon to more closely match the original macros or remove it to make the test happy?

Alternatively, the DISALLOW_COPY_AND_ASSIGN macros could have a trivial implementation that makes the trailing semicolon not "extra", like the real macro? (I guess it could be any function declaration "void stub()" for instance)

clang-tools-extra/clang-tidy/modernize/CMakeLists.txt

add_clang_library(clangTidyModernizeModule
   MakeSharedCheck.cpp
   MakeSmartPtrCheck.cpp
   MakeUniqueCheck.cpp
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp
   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp
+  ReplaceDisallowCopyAndAssignMacroCheck.cpp
   ReplaceRandomShuffleCheck.cpp
   ReturnBracedInitListCheck.cpp
   ShrinkToFitCheck.cpp
   UnaryStaticAssertCheck.cpp
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
   UseDefaultMemberInitCheck.cpp
   UseEmplaceCheck.cpp

clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "DeprecatedIosBaseAliasesCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"
 #include "MakeUniqueCheck.h"
 #include "PassByValueCheck.h"
 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
+#include "ReplaceDisallowCopyAndAssignMacroCheck.h"
 #include "ReplaceRandomShuffleCheck.h"
 #include "ReturnBracedInitListCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "UnaryStaticAssertCheck.h"
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultMemberInitCheck.h"
 #include "UseEmplaceCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MakeUniqueCheck>("modernize-make-unique");
     CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
     CheckFactories.registerCheck<RawStringLiteralCheck>(
         "modernize-raw-string-literal");
     CheckFactories.registerCheck<RedundantVoidArgCheck>(
         "modernize-redundant-void-arg");
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(
         "modernize-replace-auto-ptr");
+    CheckFactories.registerCheck<ReplaceDisallowCopyAndAssignMacroCheck>(
+        "modernize-replace-disallow-copy-and-assign-macro");
     CheckFactories.registerCheck<ReplaceRandomShuffleCheck>(
         "modernize-replace-random-shuffle");
     CheckFactories.registerCheck<ReturnBracedInitListCheck>(
         "modernize-return-braced-init-list");
     CheckFactories.registerCheck<ShrinkToFitCheck>("modernize-shrink-to-fit");
     CheckFactories.registerCheck<UnaryStaticAssertCheck>(
         "modernize-unary-static-assert");
     CheckFactories.registerCheck<UseAutoCheck>("modernize-use-auto");

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.h

+//===--- ReplaceDisallowCopyAndAssignMacroCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACEDISALLOWCOPYANDASSIGNMACROCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACEDISALLOWCOPYANDASSIGNMACROCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// This check finds macro expansions of ``DISALLOW_COPY_AND_ASSIGN(Type)`` and
+/// replaces them with a deleted copy constructor and a deleted assignment
+/// operator.
+///
+/// Before:
+/// ~~~{.cpp}
+///   class Foo {
+///   private:
+///     DISALLOW_COPY_AND_ASSIGN(Foo);
+///   };
+/// ~~~
+///
+/// After:
+/// ~~~{.cpp}
+///   class Foo {
+///   private:
+///     Foo(const Foo &) = delete;
+///     const Foo &operator=(const Foo &) = delete;
+///   };
+/// ~~~
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.html
+class ReplaceDisallowCopyAndAssignMacroCheck : public ClangTidyCheck {
+public:
+  ReplaceDisallowCopyAndAssignMacroCheck(StringRef Name,
+                                         ClangTidyContext *Context);
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus11;
+  }
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+  const std::string &getMacroName() const { return MacroName; }
+
+private:
+  const std::string MacroName;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACEDISALLOWCOPYANDASSIGNMACROCHECK_H

clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp

+//===--- ReplaceDisallowCopyAndAssignMacroCheck.cpp - clang-tidy ----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ReplaceDisallowCopyAndAssignMacroCheck.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/MacroArgs.h"
+#include "llvm/Support/FormatVariadic.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+namespace {
+
+class ReplaceDisallowCopyAndAssignMacroCallbacks : public PPCallbacks {
+public:
+  explicit ReplaceDisallowCopyAndAssignMacroCallbacks(
+      ReplaceDisallowCopyAndAssignMacroCheck &Check, Preprocessor &PP)
+      : Check(Check), PP(PP) {}
+
+  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
+                    SourceRange Range, const MacroArgs *Args) override {
+    IdentifierInfo *Info = MacroNameTok.getIdentifierInfo();
+    if (!Info || !Args || Args->getNumMacroArguments() != 1)
+      return;
+    if (Info->getName() != Check.getMacroName())
+      return;
+    // The first argument to the DISALLOW_COPY_AND_ASSIGN macro is exptected to
+    // be the class name.
+    const Token *ClassNameTok = Args->getUnexpArgument(0);
+    if (Args->ArgNeedsPreexpansion(ClassNameTok, PP))
+      // For now we only support simple argument that don't need to be
+      // pre-expanded.
+      return;
+    clang::IdentifierInfo *ClassIdent = ClassNameTok->getIdentifierInfo();
+    if (!ClassIdent)
+      return;
+
+    std::string Replacement = llvm::formatv(
+        R"cpp({0}(const {0} &) = delete;
+const {0} &operator=(const {0} &) = delete{1})cpp",
+        ClassIdent->getName(), shouldAppendSemi(Range) ? ";" : "");
+
+    Check.diag(MacroNameTok.getLocation(),
+               "prefer deleting copy constructor and assignment operator over "
+               "using macro '%0'")
+        << Check.getMacroName()
+        << FixItHint::CreateReplacement(
+               PP.getSourceManager().getExpansionRange(Range), Replacement);
+  }
+
+private:
+  /// \returns \c true if the next token after the given \p MacroLoc is \b not a
+  /// semicolon.
+  bool shouldAppendSemi(SourceRange MacroLoc) {
+    llvm::Optional<Token> Next = Lexer::findNextToken(
+        MacroLoc.getEnd(), PP.getSourceManager(), PP.getLangOpts());
+    return !(Next && Next->is(tok::semi));
+  }
+
+  ReplaceDisallowCopyAndAssignMacroCheck &Check;
+  Preprocessor &PP;
+};
+} // namespace
+
+ReplaceDisallowCopyAndAssignMacroCheck::ReplaceDisallowCopyAndAssignMacroCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      MacroName(Options.get("MacroName", "DISALLOW_COPY_AND_ASSIGN")) {}
+
+void ReplaceDisallowCopyAndAssignMacroCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(
+      ::std::make_unique<ReplaceDisallowCopyAndAssignMacroCallbacks>(
+          *this, *ModuleExpanderPP));
+}
+
+void ReplaceDisallowCopyAndAssignMacroCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "MacroName", MacroName);
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`llvmlibc-implementation-in-namespace
   Checks all llvm-libc implementation is within the correct namespace.
 
 - New :doc:`llvmlibc-restrict-system-libc-headers
   <clang-tidy/checks/llvmlibc-restrict-system-libc-headers>` check.
 
   Finds includes of system libc headers not provided by the compiler within
   llvm-libc implementations.
 
+- New :doc:`modernize-replace-disallow-copy-and-assign-macro
+  <clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro>` check.
+
+  Finds macro expansions of ``DISALLOW_COPY_AND_ASSIGN`` and replaces them with
+  a deleted copy constructor and a deleted assignment operator.
+
 - New :doc:`objc-dealloc-in-category
   <clang-tidy/checks/objc-dealloc-in-category>` check.
 
   Finds implementations of -dealloc in Objective-C categories.
 
 - New :doc:`misc-no-recursion
   <clang-tidy/checks/misc-no-recursion>` check.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `hicpp-signed-bitwise <hicpp-signed-bitwise.html>`_,
    `linuxkernel-must-use-errs <linuxkernel-must-use-errs.html>`_,
    `llvm-header-guard <llvm-header-guard.html>`_,
    `llvm-include-order <llvm-include-order.html>`_, "Yes"
    `llvm-namespace-comment <llvm-namespace-comment.html>`_,
    `llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm-prefer-isa-or-dyn-cast-in-conditionals.html>`_, "Yes"
    `llvm-prefer-register-over-unsigned <llvm-prefer-register-over-unsigned.html>`_, "Yes"
    `llvm-twine-local <llvm-twine-local.html>`_, "Yes"
    `llvmlibc-callee-namespace <llvmlibc-calle-namespace.html>`_,
    `llvmlibc-implementation-in-namespace <llvmlibc-implementation-in-namespace.html>`_,
    `llvmlibc-restrict-system-libc-headers <llvmlibc-restrict-system-libc-headers.html>`_, "Yes"
    `misc-definitions-in-headers <misc-definitions-in-headers.html>`_, "Yes"
    `misc-misplaced-const <misc-misplaced-const.html>`_,
    `misc-new-delete-overloads <misc-new-delete-overloads.html>`_,
    `misc-no-recursion <misc-no-recursion.html>`_,
    `misc-non-copyable-objects <misc-non-copyable-objects.html>`_,
    `misc-non-private-member-variables-in-classes <misc-non-private-member-variables-in-classes.html>`_,
.. csv-table::
    `modernize-deprecated-ios-base-aliases <modernize-deprecated-ios-base-aliases.html>`_, "Yes"
    `modernize-loop-convert <modernize-loop-convert.html>`_, "Yes"
    `modernize-make-shared <modernize-make-shared.html>`_, "Yes"
    `modernize-make-unique <modernize-make-unique.html>`_, "Yes"
    `modernize-pass-by-value <modernize-pass-by-value.html>`_, "Yes"
    `modernize-raw-string-literal <modernize-raw-string-literal.html>`_, "Yes"
    `modernize-redundant-void-arg <modernize-redundant-void-arg.html>`_, "Yes"
    `modernize-replace-auto-ptr <modernize-replace-auto-ptr.html>`_, "Yes"
+   `modernize-replace-disallow-copy-and-assign-macro <modernize-replace-disallow-copy-and-assign-macro.html>`_, "Yes"
    `modernize-replace-random-shuffle <modernize-replace-random-shuffle.html>`_, "Yes"
    `modernize-return-braced-init-list <modernize-return-braced-init-list.html>`_, "Yes"
    `modernize-shrink-to-fit <modernize-shrink-to-fit.html>`_, "Yes"
    `modernize-unary-static-assert <modernize-unary-static-assert.html>`_, "Yes"
    `modernize-use-auto <modernize-use-auto.html>`_, "Yes"
    `modernize-use-bool-literals <modernize-use-bool-literals.html>`_, "Yes"
    `modernize-use-default-member-init <modernize-use-default-member-init.html>`_, "Yes"
    `modernize-use-emplace <modernize-use-emplace.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst

+.. title:: clang-tidy - modernize-replace-disallow-copy-and-assign-macro
+
+modernize-replace-disallow-copy-and-assign-macro
+================================================
+
+Finds macro expansions of ``DISALLOW_COPY_AND_ASSIGN(Type)`` and replaces them
+with a deleted copy constructor and a deleted assignment operator.
+
+Before the ``delete`` keyword was introduced in C++11 it was common practice to
+declare a copy constructor and an assignment operator as a private members. This
+effectively makes them unusable to the public API of a class.
+
+With the advent of the ``delete`` keyword in C++11 we can abandon the
+``private`` access of the copy constructor and the assignment operator and
+delete the methods entirely.
+
+When running this check on a code like this:
+
+.. code-block:: c++
+
+  class Foo {
+  private:
+    DISALLOW_COPY_AND_ASSIGN(Foo);
+  };
+
+It will be transformed to this:
+
+.. code-block:: c++
+
+  class Foo {
+  private:
+    Foo(const Foo &) = delete;
+    const Foo &operator=(const Foo &) = delete;
+  };
+
+Known Limitations
+-----------------
+
+* Notice that the migration example above leaves the ``private`` access
+  specification untouched. You might want to run the check:doc:`modernize-use-equals-delete
+  <modernize-use-equals-delete>` to get warnings for deleted functions in
+  private sections.
+
+Options
+-------
+
+.. option:: MacroName
+
+   A string specifying the macro name whose expansion will be replaced.
+   Default is `DISALLOW_COPY_AND_ASSIGN`.
+
+See: https://en.cppreference.com/w/cpp/language/function#Deleted_functions

clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp

+// RUN: %check_clang_tidy -format-style=LLVM -check-suffix=DEFAULT %s \
+// RUN:   modernize-replace-disallow-copy-and-assign-macro %t
+
+// RUN: %check_clang_tidy -format-style=LLVM -check-suffix=DIFFERENT-NAME %s \
+// RUN:  modernize-replace-disallow-copy-and-assign-macro %t \
+// RUN:  -config="{CheckOptions: [ \
+// RUN:   {key: modernize-replace-disallow-copy-and-assign-macro.MacroName, \
+// RUN:    value: MY_MACRO_NAME}]}"
+
+// RUN: %check_clang_tidy -format-style=LLVM -check-suffix=FINALIZE %s \
+// RUN:  modernize-replace-disallow-copy-and-assign-macro %t \
+// RUN:  -config="{CheckOptions: [ \
+// RUN:   {key: modernize-replace-disallow-copy-and-assign-macro.MacroName, \
+// RUN:    value: DISALLOW_COPY_AND_ASSIGN_FINALIZE}]}"
+
+// RUN: clang-tidy %s -checks="-*,modernize-replace-disallow-copy-and-assign-macro" \
+// RUN:   -config="{CheckOptions: [ \
+// RUN:    {key: modernize-replace-disallow-copy-and-assign-macro.MacroName, \
+// RUN:     value: DISALLOW_COPY_AND_ASSIGN_MORE_AGUMENTS}]}" | count 0
+
+// RUN: clang-tidy %s -checks="-*,modernize-replace-disallow-copy-and-assign-macro" \
+// RUN:   -config="{CheckOptions: [ \
+// RUN:    {key: modernize-replace-disallow-copy-and-assign-macro.MacroName, \
+// RUN:     value: DISALLOW_COPY_AND_ASSIGN_NEEDS_PREEXPANSION}]}" | count 0
+
+// Note: the last two tests expect no diagnostics, but FileCheck cannot handle
+// that, hence the use of | count 0.
+
+#define DISALLOW_COPY_AND_ASSIGN(TypeName)
+
+class TestClass1 {
+private:
+  DISALLOW_COPY_AND_ASSIGN(TestClass1);
+};
+// CHECK-MESSAGES-DEFAULT: :[[@LINE-2]]:3: warning: prefer deleting copy constructor and assignment operator over using macro 'DISALLOW_COPY_AND_ASSIGN' [modernize-replace-disallow-copy-and-assign-macro]
+// CHECK-FIXES-DEFAULT:      {{^}}  TestClass1(const TestClass1 &) = delete;{{$}}
+// CHECK-FIXES-DEFAULT-NEXT: {{^}}  const TestClass1 &operator=(const TestClass1 &) = delete;{{$}}
+
+#define MY_MACRO_NAME(TypeName)
+
+class TestClass2 {
+private:
+  MY_MACRO_NAME(TestClass2);
+};
+// CHECK-MESSAGES-DIFFERENT-NAME: :[[@LINE-2]]:3: warning: prefer deleting copy constructor and assignment operator over using macro 'MY_MACRO_NAME' [modernize-replace-disallow-copy-and-assign-macro]
+// CHECK-FIXES-DIFFERENT-NAME:      {{^}}  TestClass2(const TestClass2 &) = delete;{{$}}
+// CHECK-FIXES-DIFFERENT-NAME-NEXT: {{^}}  const TestClass2 &operator=(const TestClass2 &) = delete;{{$}}
+
+#define DISALLOW_COPY_AND_ASSIGN_FINALIZE(TypeName) \
+  TypeName(const TypeName &) = delete;              \
+  const TypeName &operator=(const TypeName &) = delete;
+
+class TestClass3 {
+private:
+  // Notice, that the macro allows to be used without a semicolon because the
+  // macro definition already contains one above. Therefore our replacement must
+  // contain a semicolon at the end.
+  DISALLOW_COPY_AND_ASSIGN_FINALIZE(TestClass3)
+};
+// CHECK-MESSAGES-FINALIZE: :[[@LINE-2]]:3: warning: prefer deleting copy constructor and assignment operator over using macro 'DISALLOW_COPY_AND_ASSIGN_FINALIZE' [modernize-replace-disallow-copy-and-assign-macro]
+// CHECK-FIXES-FINALIZE:      {{^}}  TestClass3(const TestClass3 &) = delete;{{$}}
+// CHECK-FIXES-FINALIZE-NEXT: {{^}}  const TestClass3 &operator=(const TestClass3 &) = delete;{{$}}
+
+#define DISALLOW_COPY_AND_ASSIGN_MORE_AGUMENTS(A, B)
+
+class TestClass4 {
+private:
+  DISALLOW_COPY_AND_ASSIGN_MORE_AGUMENTS(TestClass4, TestClass4);
+};
+// CHECK-MESSAGES-MORE-ARGUMENTS-NOT: warning: prefer deleting copy constructor and assignment operator over using macro 'DISALLOW_COPY_AND_ASSIGN_MORE_AGUMENTS'
+
+#define DISALLOW_COPY_AND_ASSIGN_NEEDS_PREEXPANSION(A)
+#define TESTCLASS TestClass5
+
+class TestClass5 {
+private:
+  DISALLOW_COPY_AND_ASSIGN_NEEDS_PREEXPANSION(TESTCLASS);
+};
+// CHECK-MESSAGES-MORE-ARGUMENTS-NOT: warning: prefer deleting copy constructor and assignment operator over using macro 'DISALLOW_COPY_AND_ASSIGN_NEEDS_PREEXPANSION'