[clang-tidy] Special member functions check should allow sole dtors by default

[steakhal]

Summary

The cppcoreguidelines-special-member-functions check has the AllowSoleDefaultDtor checker option.
It is false by default, and now I'm proposing to change it to true.
Our CodeChecker users frequently find it surprising that the check reports for cases like this by default:

struct A {
  virtual ~A() = default;
};

---

Quoting from the Cpp CoreGuidelines C.21: If you define or =delete any copy, move, or destructor function, define or =delete them all:

Example, good When a destructor needs to be declared just to make it virtual, it can be defined as defaulted.

class AbstractBase {
public:
    virtual ~AbstractBase() = default;
    // ...
};

[aaron.ballman]

I think the C++ Core Guideline wording is... confusing. The rule title is C.21: If you define or =delete any copy, move, or destructor function, define or =delete them all, which I take literally. Using = default defines the function, so:

struct S {
  virtual ~S() = default; // This defines the dtor
};

Because this defines the dtor, all the rest need to be defined as well per the rule. The enforcement on the rule says: Enforcement (Simple) A class should have a declaration (even a =delete one) for either all or none of the copy/move/destructor functions. and so it seems like the rule is requiring us to diagnose this case by default (pun retroactively intended), and so the option should remain as it is. In the code example you posted from the rule, I imagine that to be a load-bearing // ... that defines all the rest of the members.

Perhaps the C++ Core Guideline authors can be enticed to update the rule to be more clear?

[steakhal]

In D103511#2793291, @aaron.ballman wrote:

I think the C++ Core Guideline wording is... confusing. The rule title is C.21: If you define or =delete any copy, move, or destructor function, define or =delete them all, which I take literally. Using = default defines the function, so:

Okay, then I'm going to abandon this in a few days if nothing interesting shows up.
Thanks for the quick response!

[steakhal]

I forgot to abandon this earlier.

clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 SpecialMemberFunctionsCheck::SpecialMemberFunctionsCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), AllowMissingMoveFunctions(Options.get(
                                          "AllowMissingMoveFunctions", false)),
-      AllowSoleDefaultDtor(Options.get("AllowSoleDefaultDtor", false)),
+      AllowSoleDefaultDtor(Options.get("AllowSoleDefaultDtor", true)),
       AllowMissingMoveFunctionsWhenCopyIsDeleted(
           Options.get("AllowMissingMoveFunctionsWhenCopyIsDeleted", false)) {}
 
 void SpecialMemberFunctionsCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "AllowMissingMoveFunctions", AllowMissingMoveFunctions);
   Options.store(Opts, "AllowSoleDefaultDtor", AllowSoleDefaultDtor);
   Options.store(Opts, "AllowMissingMoveFunctionsWhenCopyIsDeleted",

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst

 
 https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all.
 
 Options
 -------
 
 .. option:: AllowSoleDefaultDtor
 
-   When set to `true` (default is `false`), this check doesn't flag classes with a sole, explicitly
+   When set to `true` (default is `true`), this check doesn't flag classes with a sole, explicitly
    defaulted destructor. An example for such a class is:
    
    .. code-block:: c++
    
      struct A {
        virtual ~A() = default;
      };
    

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-special-member-functions-allow-missing-move-when-copy-is-deleted.cpp

 };
 
 // CHECK-MESSAGES: [[@LINE+1]]:7:  warning: class 'MissingCopyOperator' defines a default destructor and a copy constructor but does not define a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
 class MissingCopyOperator {
   ~MissingCopyOperator() = default;
   MissingCopyOperator(const MissingCopyOperator &) = delete;
 };
 
-// CHECK-MESSAGES: [[@LINE+1]]:7:  warning: class 'MissingAll' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
 class MissingAll {
   ~MissingAll() = default;
 };

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-special-member-functions.cpp

-// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t
-
+// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: [{key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor, value: false}]}" --
 class DefinesDestructor {
   ~DefinesDestructor();
 };
 // CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
 
 class DefinesDefaultedDestructor {
   ~DefinesDefaultedDestructor() = default;
 };