[clang-tidy] Handle unions in modernize-use-equals-default check

[malcolm.parsons]

Summary

Do not replace the constructor or destructor of a union with =default
if it would be implicitly deleted.

Fixes: PR27926

[aaron.ballman]

clang-tidy/modernize/UseEqualsDefaultCheck.cpp

117

Please don't use auto here or elsewhere if the type is not explicitly spelled out.

119–120

This does not match what's in [class.ctor]p5.

"X is a union that has a variant member with a non-trivial default constructor and no variant member of X has a default member initializer" -- you aren't checking for the default member initializer.

also missing for unions: "X is a union and all of its variant members are of const-qualified type (or array thereof)"

(You should add test cases for these.)

It might make more sense to implement this as CXXRecordDecl::defaultedDestructorIsDeleted()?

127

You can use auto here and below.

[malcolm.parsons]

clang-tidy/modernize/UseEqualsDefaultCheck.cpp

117

This was copied from isCopyAssignmentAndCanBeDefaulted().

119–120

Is there any way to call Sema::ShouldDeleteSpecialMember() from a clang-tidy check?

127

This was copied from CXXRecordDecl::addedMember().

[alexfh]

clang-tidy/modernize/UseEqualsDefaultCheck.cpp

119–120

By the time clang-tidy checks are invoked, Sema is already dead, and I don't think there's a reasonable way to use it.

127

Not all LLVM/Clang code is ideal. It makes sense to clean up issues once the code is changed and even more so when it's copied elsewhere.

clang-tidy/modernize/UseEqualsDefaultCheck.cpp

if (match(
       return false;
   }
 
   // Ensure that we don't do anything else, like initializing an indirect base.
   return Ctor->getNumCtorInitializers() ==
          BasesToInit.size() + FieldsToInit.size();
 }
 
+/// Checks that the given default constructor can be defaulted.
+static bool defaultConstructorCanBeDefaulted(ASTContext *Context,
+                                             const CXXMethodDecl *Operator) {
+  const auto *Record = Operator->getParent();
+
+  // A defaulted default constructor of a union with a field with a non trivial
+  // default constructor would be deleted.
+  if (Record->isUnion()) {
+    auto FieldsToInit = getAllNamedFields(Record);
+
+    for (const auto *Field : FieldsToInit) {
+      QualType T = Context->getBaseElementType(Field->getType());
+
+      if (const RecordType *RecordTy = T->getAs<RecordType>()) {
+        CXXRecordDecl *FieldRec = cast<CXXRecordDecl>(RecordTy->getDecl());
+
+        if (FieldRec->hasNonTrivialDefaultConstructor())
+          return false;
+      }
+    }
+  }
+
+  return true;
+}
+
+/// Checks that the given destructor can be defaulted.
+static bool destructorCanBeDefaulted(ASTContext *Context,
+                                     const CXXMethodDecl *Operator) {
+  const auto *Record = Operator->getParent();
+
+  // A defaulted destructor of a union with a field with a non trivial
+  // destructor would be deleted.
+  if (Record->defaultedDestructorIsDeleted())
+    return false;
+
+  return true;
+}
+
 /// \brief Checks that the given method is an overloading of the assignment
 /// operator, has copy signature, returns a reference to "*this" and copies
 /// all its members and subobjects.
 static bool isCopyAssignmentAndCanBeDefaulted(ASTContext *Context,
                                               const CXXMethodDecl *Operator) {
   const auto *Record = Operator->getParent();
   const auto *Param = Operator->getParamDecl(0);
 
void UseEqualsDefaultCheck::check(const MatchFinder::MatchResult &Result) {
   // If there are comments inside the body, don't do the change.
   bool ApplyFix = SpecialFunctionDecl->isCopyAssignmentOperator() ||
                   bodyEmpty(Result.Context, Body);
 
   std::vector<FixItHint> RemoveInitializers;
 
   if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(SpecialFunctionDecl)) {
     if (Ctor->getNumParams() == 0) {
+      if (!defaultConstructorCanBeDefaulted(Result.Context, Ctor))
+        return;
       SpecialFunctionName = "default constructor";
     } else {
       if (!isCopyConstructorAndCanBeDefaulted(Result.Context, Ctor))
         return;
       SpecialFunctionName = "copy constructor";
       // If there are constructor initializers, they must be removed.
       for (const auto *Init : Ctor->inits()) {
         RemoveInitializers.emplace_back(
             FixItHint::CreateRemoval(Init->getSourceRange()));
       }
     }
   } else if (isa<CXXDestructorDecl>(SpecialFunctionDecl)) {
+    if (!destructorCanBeDefaulted(Result.Context, SpecialFunctionDecl))
+      return;
     SpecialFunctionName = "destructor";
   } else {
     if (!isCopyAssignmentAndCanBeDefaulted(Result.Context, SpecialFunctionDecl))
       return;
     SpecialFunctionName = "copy-assignment operator";
   }
 
   // The location of the body is more useful inside a macro as spelling and

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

 
 #define STRUCT_WITH_DEFAULT(_base, _type) \
   struct _type {                          \
     _type() {}                            \
     _base value;                          \
   };
 
 STRUCT_WITH_DEFAULT(unsigned char, InMacro)
+
+// Unions
+union UnionOfPOD {
+  UnionOfPOD() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
+  // CHECK-FIXES: UnionOfPOD() = default;
+  ~UnionOfPOD() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
+  // CHECK-FIXES: ~UnionOfPOD() = default;
+  int i;
+  double j;
+};
+
+union UnionOfNonTrivial {
+  UnionOfNonTrivial() {}
+  ~UnionOfNonTrivial() {}
+  int i;
+  double j;
+  NE k;
+};