[PATCH] D38179: [clang-tidy] Handle unions in modernize-use-equals-default check
alexfh added inline comments.
Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:119-120
+
+ // A defaulted default constructor of a union with a field with a non trivial
+ // default constructor would be deleted.
+ if (Record->isUnion()) {
malcolm.parsons wrote:
> aaron.ballman wrote:
> > 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()`?
> Is there any way to call `Sema::ShouldDeleteSpecialMember()` from a
> clang-tidy check?
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.
Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:127
+
+ if (const RecordType *RecordTy = T->getAs()) {
+CXXRecordDecl *FieldRec = cast(RecordTy->getDecl());
malcolm.parsons wrote:
> aaron.ballman wrote:
> > You can use `auto` here and below.
> This was copied from `CXXRecordDecl::addedMember()`.
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.
https://reviews.llvm.org/D38179
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38179: [clang-tidy] Handle unions in modernize-use-equals-default check
malcolm.parsons added inline comments.
Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:117
+ const CXXMethodDecl *Operator) {
+ const auto *Record = Operator->getParent();
+
aaron.ballman wrote:
> Please don't use `auto` here or elsewhere if the type is not explicitly
> spelled out.
This was copied from `isCopyAssignmentAndCanBeDefaulted()`.
Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:119-120
+
+ // A defaulted default constructor of a union with a field with a non trivial
+ // default constructor would be deleted.
+ if (Record->isUnion()) {
aaron.ballman wrote:
> 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()`?
Is there any way to call `Sema::ShouldDeleteSpecialMember()` from a clang-tidy
check?
Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:127
+
+ if (const RecordType *RecordTy = T->getAs()) {
+CXXRecordDecl *FieldRec = cast(RecordTy->getDecl());
aaron.ballman wrote:
> You can use `auto` here and below.
This was copied from `CXXRecordDecl::addedMember()`.
https://reviews.llvm.org/D38179
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38179: [clang-tidy] Handle unions in modernize-use-equals-default check
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:117
+ const CXXMethodDecl *Operator) {
+ const auto *Record = Operator->getParent();
+
Please don't use `auto` here or elsewhere if the type is not explicitly spelled
out.
Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:119-120
+
+ // A defaulted default constructor of a union with a field with a non trivial
+ // default constructor would be deleted.
+ if (Record->isUnion()) {
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()`?
Comment at: clang-tidy/modernize/UseEqualsDefaultCheck.cpp:127
+
+ if (const RecordType *RecordTy = T->getAs()) {
+CXXRecordDecl *FieldRec = cast(RecordTy->getDecl());
You can use `auto` here and below.
https://reviews.llvm.org/D38179
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38179: [clang-tidy] Handle unions in modernize-use-equals-default check
malcolm.parsons created this revision.
Herald added subscribers: xazax.hun, JDevlieghere.
Do not replace the constructor or destructor of a union with =default
if it would be implicitly deleted.
Fixes: PR27926
https://reviews.llvm.org/D38179
Files:
clang-tidy/modernize/UseEqualsDefaultCheck.cpp
test/clang-tidy/modernize-use-equals-default.cpp
Index: test/clang-tidy/modernize-use-equals-default.cpp
===
--- test/clang-tidy/modernize-use-equals-default.cpp
+++ test/clang-tidy/modernize-use-equals-default.cpp
@@ -205,3 +205,23 @@
};
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;
+};
Index: clang-tidy/modernize/UseEqualsDefaultCheck.cpp
===
--- clang-tidy/modernize/UseEqualsDefaultCheck.cpp
+++ clang-tidy/modernize/UseEqualsDefaultCheck.cpp
@@ -111,6 +111,44 @@
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()) {
+CXXRecordDecl *FieldRec = cast(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.
@@ -274,6 +312,8 @@
if (const auto *Ctor = dyn_cast(SpecialFunctionDecl)) {
if (Ctor->getNumParams() == 0) {
+ if (!defaultConstructorCanBeDefaulted(Result.Context, Ctor))
+return;
SpecialFunctionName = "default constructor";
} else {
if (!isCopyConstructorAndCanBeDefaulted(Result.Context, Ctor))
@@ -286,6 +326,8 @@
}
}
} else if (isa(SpecialFunctionDecl)) {
+if (!destructorCanBeDefaulted(Result.Context, SpecialFunctionDecl))
+ return;
SpecialFunctionName = "destructor";
} else {
if (!isCopyAssignmentAndCanBeDefaulted(Result.Context,
SpecialFunctionDecl))
Index: test/clang-tidy/modernize-use-equals-default.cpp
===
--- test/clang-tidy/modernize-use-equals-default.cpp
+++ test/clang-tidy/modernize-use-equals-default.cpp
@@ -205,3 +205,23 @@
};
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;
+};
Index: clang-tidy/modernize/UseEqualsDefaultCheck.cpp
===
--- clang-tidy/modernize/UseEqualsDefaultCheck.cpp
+++ clang-tidy/modernize/UseEqualsDefaultCheck.cpp
@@ -111,6 +111,44 @@
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
