[clang-tidy] Add modernize-use-equals-delete check

[malcolm.parsons]

Summary

Fixes PR27872

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[malcolm.parsons]

Add to release notes.

[alexfh]

"Use delete" makes me think of the delete operator. I'd suggest one of these names:

• modernize-use-equals-delete
• modernize-use-deleted-special-members
• modernize-delete-special-members

(feel free to suggest a better alternative).

clang-tidy/modernize/UseDeleteCheck.cpp

37 ↗

(On Diff #76374)

The double negation makes this part hard to parse. Can you add a comment saying (roughly) that the matcher ensures that all non-special functions declared in this class are defined in some way in the translation unit we're looking at?

[malcolm.parsons]

In D26138#583561, @alexfh wrote:

"Use delete" makes me think of the delete operator. I'd suggest one of these names:

I chose modernize-use-delete as there's already modernize-use-default.
"Use default" makes me think of the default case in a switch.

• modernize-use-equals-delete

Works for me, but should I rename modernize-use-default?

[malcolm.parsons]

Rename to modernize-use-equals-delete.
Add comment.

[malcolm.parsons]

Sort headers and CMakeLists.txt.

[Eugene.Zelenko]

It'll be good idea to rename modernize-use-default in similar fashion, since both checks are closely relate and in both cases keywords are ambiguous.

[aaron.ballman]

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

29 ↗

(On Diff #76433)

How about a conversion operator, like operator bool()? You'll sometimes see that one declared privately for similar reasons.

37 ↗

(On Diff #76433)

Missing a full stop at the end of the comment.

46 ↗

(On Diff #76433)

Should rename this to not hide the global SpecialFunction object, and use the global SpecialFunction object in place of the string literal here. Alternatively, leave this name alone and remove the global variable and just use the string literal; either is fine.

52 ↗

(On Diff #76433)

This diagnostic isn't very clear to me -- what does it mean to "prevent" a default special member function?

The fixit for this is also somewhat unsatisfying as this takes a private, not-defined function and turns it into a private, deleted function. That's a very small win, because it only impacts code which could access the special member function in the first place (some compilers give a diagnostic about the special member function being inaccessible even if it's explicitly marked as deleted; clang is not one such compiler). Do we have a way to rewrite the access specifier for the special member function as well (kind of like how we have a way to handle includes we're adding)? I am guessing not yet, but if we do, that would be fantastic to use here.

Note, I don't think this should hold up your patch or the fixit. A small win is still a win. :-)

test/clang-tidy/modernize-use-equals-delete.cpp

93 ↗

(On Diff #76433)

Phab will not let me delete the unsubmitted comment I had about this, so I am leaving a junk comment instead. Your move, Phabricator.

[malcolm.parsons]

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

29 ↗

(On Diff #76433)

I haven't seen that. Do you have an example?

52 ↗

(On Diff #76433)

Do you have a better wording for the diagnostic?

I don't see any utility functions to make a method public.

[malcolm.parsons]

Fix comment.
Rename variable.
Use global.

[malcolm.parsons]

Rename modernize-use-default to modernize-use-equals-default

[aaron.ballman]

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

29 ↗

(On Diff #76433)

anecdote != data, and all that, but: http://stackoverflow.com/questions/5753460/a-way-to-disable-conversion-operators

I do agree though, this is not as common as noncopyable classes.

52 ↗

(On Diff #76433)

Perhaps: "special member function with private access specifier and no definition is still accessible; use '= delete' to explicitly disallow all access"?

Or a less-wordy variant.

[malcolm.parsons]

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

29 ↗

(On Diff #76433)

I think I'll leave conversion operators to future modernize-use-explicit-conversion-operators or cppcoreguidelines-avoid-conversion-operators checks.

52 ↗

(On Diff #76433)

How about "use '= delete' to prohibit calling of a special member function".

[aaron.ballman]

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

29 ↗

(On Diff #76433)

I'm okay with that.

52 ↗

(On Diff #76433)

Given that this is in the modernize module, I think that's reasonable wording.

[malcolm.parsons]

Reword diagnostic.

[malcolm.parsons]

Add FIXME comment.

[malcolm.parsons]

Add quick hack to make methods public.
Cleanup around replacements should be enhanced with rules to remove
empty private/protected/public sections.
private: public: -> public:
public: private: -> private:

[aaron.ballman]

Can you split the modernize-use-default changes into a separate patch? I think that change requires additional discussion, because renaming the check may break users relying on the old name. The changes aren't really related (aside from naming consistency), and I'd rather not let modernize-use-default discussion delay accepting this patch.

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

58 ↗

(On Diff #76561)

I am on the fence about this fixit. On the one hand, the fix is a technical improvement because it means that implementations will consistently find the declaration and bark about it being explicitly deleted. On the other hand, the fixit suggests code that should never pass a reasonable code review.

I'm wondering if it would make more sense to leave the access specifiers alone and just put a FIXME in the code to point the situation out. I am guessing that at some point we will have a refactoring tool that can help without resorting to making declarations like public: C() = delete; private:.

What do you think?

[malcolm.parsons]

Take out renaming of modernize-use-default.

[malcolm.parsons]

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

58 ↗

(On Diff #76561)

I'm wondering whether no fixit would be better than a not-good-enough fixit.

[aaron.ballman]

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

58 ↗

(On Diff #76561)

Generally, no. Incremental improvements are almost always fine. However, the user is asking to have their code modernized, and the fixit results in code that looks more foreign than modern (at least, to me).

I won't block the patch moving forward regardless of whether the fixit is in or out, but I am curious if @alexfh has an opinion, or if you have a strong preference one way or the other.

[malcolm.parsons]

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

58 ↗

(On Diff #76561)

I'll change the check to warn about deleted methods that aren't public; the user can fix those manually until a better fixit is possible.

[malcolm.parsons]

Check for non-public deleted methods.
Remove imperfect FixItHint.
Add FIXME comments.

[aaron.ballman]

LGTM, thank you!

clang-tidy/modernize/UseEqualsDeleteCheck.cpp

58 ↗

(On Diff #76561)

I think that's a good solution for now, thanks!

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

add_clang_library(clangTidyModernizeModule
   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp
   ShrinkToFitCheck.cpp
   UseAutoCheck.cpp
   UseBoolLiteralsCheck.cpp
   UseDefaultCheck.cpp
   UseEmplaceCheck.cpp
+  UseEqualsDeleteCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
   UseUsingCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyReadabilityModule
   clangTidyUtils
   )

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

 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "UseAutoCheck.h"
 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultCheck.h"
 #include "UseEmplaceCheck.h"
+#include "UseEqualsDeleteCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 #include "UseUsingCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(
         "modernize-replace-auto-ptr");
     CheckFactories.registerCheck<ShrinkToFitCheck>("modernize-shrink-to-fit");
     CheckFactories.registerCheck<UseAutoCheck>("modernize-use-auto");
     CheckFactories.registerCheck<UseBoolLiteralsCheck>(
         "modernize-use-bool-literals");
     CheckFactories.registerCheck<UseDefaultCheck>("modernize-use-default");
     CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
+    CheckFactories.registerCheck<UseEqualsDeleteCheck>(
+        "modernize-use-equals-delete");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
     CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     auto &Opts = Options.CheckOptions;

clang-tools-extra/trunk/clang-tidy/modernize/UseEqualsDeleteCheck.h

+//===--- UseEqualsDeleteCheck.h - clang-tidy---------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EQUALS_DELETE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EQUALS_DELETE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// \brief Mark unimplemented private special member functions with '= delete'.
+/// \code
+///   struct A {
+///   private:
+///     A(const A&);
+///     A& operator=(const A&);
+///   };
+/// \endcode
+/// Is converted to:
+/// \code
+///   struct A {
+///   private:
+///     A(const A&) = delete;
+///     A& operator=(const A&) = delete;
+///   };
+/// \endcode
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html
+class UseEqualsDeleteCheck : public ClangTidyCheck {
+public:
+  UseEqualsDeleteCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_EQUALS_DELETE_H

clang-tools-extra/trunk/clang-tidy/modernize/UseEqualsDeleteCheck.cpp

+//===--- UseEqualsDeleteCheck.cpp - clang-tidy-----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseEqualsDeleteCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+static const char SpecialFunction[] = "SpecialFunction";
+static const char DeletedNotPublic[] = "DeletedNotPublic";
+
+void UseEqualsDeleteCheck::registerMatchers(MatchFinder *Finder) {
+  auto PrivateSpecialFn = cxxMethodDecl(
+      isPrivate(),
+      anyOf(cxxConstructorDecl(anyOf(isDefaultConstructor(),
+                                     isCopyConstructor(), isMoveConstructor())),
+            cxxMethodDecl(
+                anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator())),
+            cxxDestructorDecl()));
+
+  Finder->addMatcher(
+      cxxMethodDecl(
+          PrivateSpecialFn,
+          unless(anyOf(hasBody(stmt()), isDefaulted(), isDeleted(),
+                       // Ensure that all methods except private special member
+                       // functions are defined.
+                       hasParent(cxxRecordDecl(hasMethod(unless(
+                           anyOf(PrivateSpecialFn, hasBody(stmt()), isPure(),
+                                 isDefaulted(), isDeleted()))))))))
+          .bind(SpecialFunction),
+      this);
+
+  Finder->addMatcher(
+      cxxMethodDecl(isDeleted(), unless(isPublic())).bind(DeletedNotPublic),
+      this);
+}
+
+void UseEqualsDeleteCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Func =
+          Result.Nodes.getNodeAs<CXXMethodDecl>(SpecialFunction)) {
+    SourceLocation EndLoc = Lexer::getLocForEndOfToken(
+        Func->getLocEnd(), 0, *Result.SourceManager, getLangOpts());
+
+    // FIXME: Improve FixItHint to make method public
+    diag(Func->getLocation(),
+         "use '= delete' to prohibit calling of a special member function")
+        << FixItHint::CreateInsertion(EndLoc, " = delete");
+  } else if (const auto *Func =
+                 Result.Nodes.getNodeAs<CXXMethodDecl>(DeletedNotPublic)) {
+    // FIXME: Add FixItHint to make method public
+    diag(Func->getLocation(), "deleted member function should be public");
+  }
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- `modernize-make-unique
   and `modernize-make-shared
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html>`_
   now handle calls to the smart pointer's ``reset()`` method.
 
 - The `modernize-use-auto
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html>`_ check
   now warns about variable declarations that are initialized with a cast.
 
+- New `modernize-use-equals-delete
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html>`_ check
+
+  Adds ``= delete`` to unimplemented private special member functions.
+
 - New `mpi-buffer-deref
   <http://clang.llvm.org/extra/clang-tidy/checks/mpi-buffer-deref.html>`_ check
 
   Flags buffers which are insufficiently dereferenced when passed to an MPI function call.
 
 - New `mpi-type-mismatch
   <http://clang.llvm.org/extra/clang-tidy/checks/mpi-type-mismatch.html>`_ check
 

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

.. toctree::
    modernize-raw-string-literal
    modernize-redundant-void-arg
    modernize-replace-auto-ptr
    modernize-shrink-to-fit
    modernize-use-auto
    modernize-use-bool-literals
    modernize-use-default
    modernize-use-emplace
+   modernize-use-equals-delete
    modernize-use-nullptr
    modernize-use-override
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-cast-in-loop

clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-default.rst

.. code-block:: c++
 
   struct A {
     A() = default;
     ~A();
   };
   A::~A() = default;
 
 .. note::
-  Copy-constructor, copy-assignment operator, move-constructor and
-  move-assignment operator are not supported yet.
+  Move-constructor and move-assignment operator are not supported yet.

clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-equals-delete.rst

+.. title:: clang-tidy - modernize-use-equals-delete
+
+modernize-use-equals-delete
+===========================
+
+This check marks unimplemented private special member functions with ``= delete``.
+To avoid false-positives, this check only applies in a translation unit that has
+all other member functions implemented.
+
+.. code-block:: c++
+
+  struct A {
+  private:
+    A(const A&);
+    A& operator=(const A&);
+  };
+
+  // becomes
+
+  struct A {
+  private:
+    A(const A&) = delete;
+    A& operator=(const A&) = delete;
+  };
+

clang-tools-extra/trunk/test/clang-tidy/modernize-use-equals-delete.cpp

+// RUN: %check_clang_tidy %s modernize-use-equals-delete %t
+
+struct PositivePrivate {
+private:
+  PositivePrivate();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositivePrivate() = delete;
+  PositivePrivate(const PositivePrivate &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositivePrivate(const PositivePrivate &) = delete;
+  PositivePrivate &operator=(const PositivePrivate &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositivePrivate &operator=(const PositivePrivate &) = delete;
+  PositivePrivate(PositivePrivate &&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositivePrivate(PositivePrivate &&) = delete;
+  PositivePrivate &operator=(PositivePrivate &&);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositivePrivate &operator=(PositivePrivate &&) = delete;
+  ~PositivePrivate();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: ~PositivePrivate() = delete;
+};
+
+struct NegativePublic {
+  NegativePublic(const NegativePublic &);
+};
+
+struct NegativeProtected {
+protected:
+  NegativeProtected(const NegativeProtected &);
+};
+
+struct PositiveInlineMember {
+  int foo() { return 0; }
+
+private:
+  PositiveInlineMember(const PositiveInlineMember &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositiveInlineMember(const PositiveInlineMember &) = delete;
+};
+
+struct PositiveOutOfLineMember {
+  int foo();
+
+private:
+  PositiveOutOfLineMember(const PositiveOutOfLineMember &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositiveOutOfLineMember(const PositiveOutOfLineMember &) = delete;
+};
+
+int PositiveOutOfLineMember::foo() { return 0; }
+
+struct PositiveAbstractMember {
+  virtual int foo() = 0;
+
+private:
+  PositiveAbstractMember(const PositiveAbstractMember &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositiveAbstractMember(const PositiveAbstractMember &) = delete;
+};
+
+struct NegativeMemberNotImpl {
+  int foo();
+
+private:
+  NegativeMemberNotImpl(const NegativeMemberNotImpl &);
+};
+
+struct NegativeStaticMemberNotImpl {
+  static int foo();
+
+private:
+  NegativeStaticMemberNotImpl(const NegativeStaticMemberNotImpl &);
+};
+
+struct NegativeInline {
+private:
+  NegativeInline(const NegativeInline &) {}
+};
+
+struct NegativeOutOfLine {
+private:
+  NegativeOutOfLine(const NegativeOutOfLine &);
+};
+
+NegativeOutOfLine::NegativeOutOfLine(const NegativeOutOfLine &) {}
+
+struct NegativeConstructNotImpl {
+  NegativeConstructNotImpl();
+
+private:
+  NegativeConstructNotImpl(const NegativeConstructNotImpl &);
+};
+
+struct PositiveDefaultedConstruct {
+  PositiveDefaultedConstruct() = default;
+
+private:
+  PositiveDefaultedConstruct(const PositiveDefaultedConstruct &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositiveDefaultedConstruct(const PositiveDefaultedConstruct &) = delete;
+};
+
+struct PositiveDeletedConstruct {
+  PositiveDeletedConstruct() = delete;
+
+private:
+  PositiveDeletedConstruct(const PositiveDeletedConstruct &);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= delete' to prohibit calling of a special member function [modernize-use-equals-delete]
+  // CHECK-FIXES: PositiveDeletedConstruct(const PositiveDeletedConstruct &) = delete;
+};
+
+struct NegativeDefaulted {
+private:
+  NegativeDefaulted(const NegativeDefaulted &) = default;
+};
+
+struct PrivateDeleted {
+private:
+  PrivateDeleted(const PrivateDeleted &) = delete;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: deleted member function should be public [modernize-use-equals-delete]
+};
+
+struct ProtectedDeleted {
+protected:
+  ProtectedDeleted(const ProtectedDeleted &) = delete;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: deleted member function should be public [modernize-use-equals-delete]
+};
+
+struct PublicDeleted {
+public:
+  PublicDeleted(const PublicDeleted &) = delete;
+};