Page MenuHomePhabricator

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

Authored by kwk on May 25 2020, 12:30 PM.

Details

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst
45

Please use single back-ticks for option values.

49

Please use single back-ticks for option values.

Eugene.Zelenko added a project: Restricted Project.
kwk updated this revision to Diff 266100.May 25 2020, 9:59 PM
kwk marked 8 inline comments as done.
  • 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 added a comment.May 25 2020, 9:59 PM

@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 updated this revision to Diff 266101.May 25 2020, 10:02 PM
  • Move comment about FinalizeWithSemicolon into code
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 planned changes to this revision.May 26 2020, 2:30 AM

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

kwk updated this revision to Diff 266148.May 26 2020, 3:09 AM
kwk marked 4 inline comments as done.
  • 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 marked 2 inline comments as done.May 26 2020, 3:11 AM

@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 added inline comments.May 26 2020, 4:33 AM
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 added inline comments.May 26 2020, 6:22 AM
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 updated this revision to Diff 266251.May 26 2020, 9:44 AM
kwk marked 11 inline comments as done.
  • 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 added a comment.May 26 2020, 9:44 AM

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 added inline comments.May 26 2020, 9:46 AM
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 updated this revision to Diff 266257.May 26 2020, 9:57 AM
kwk marked 2 inline comments as done.
  • 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.

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 marked an inline comment as done.May 26 2020, 10:32 AM
kwk added inline comments.
clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst
42
kwk updated this revision to Diff 266268.May 26 2020, 10:35 AM
  • Adjust link to documentation
kwk marked 2 inline comments as done.May 26 2020, 10:36 AM
kwk added inline comments.
clang-tools-extra/docs/clang-tidy/checks/modernize-replace-disallow-copy-and-assign-macro.rst
42
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 updated this revision to Diff 266438.May 27 2020, 12:12 AM
kwk marked an inline comment as done.
  • fix link in doc
kwk added a comment.May 27 2020, 12:12 AM

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

kwk added a comment.May 28 2020, 5:12 AM

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

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 updated this revision to Diff 267021.May 28 2020, 1:30 PM
kwk marked 9 inline comments as done.
  • 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 added a comment.May 28 2020, 1:31 PM

@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 4 inline comments as done.Jun 2 2020, 9:17 AM

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 accepted this revision.Jun 2 2020, 12:36 PM

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.

This revision is now accepted and ready to land.Jun 2 2020, 12:36 PM
kwk updated this revision to Diff 267960.Jun 2 2020, 1:02 PM
kwk marked 4 inline comments as done.
  • Return macro name by reference
  • Add colon to paragraph showing code
kwk added a comment.Jun 2 2020, 1:02 PM

Done.

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

Yeah, that makes sense. Stupid me.

kwk added a comment.Jun 2 2020, 2:05 PM

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?

This revision was automatically updated to reflect the committed changes.