[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
modocache abandoned this revision. modocache added a comment. Yes, thanks @rsmith! And sorry @ahatanak for the trouble of explaining your code here. The new standards behavior does impact @twoh and I's codebase in a few places, but as you explained we can simply change the source to not use list-initialization. Thanks all! Repository: rC Clang https://reviews.llvm.org/D53860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
twoh added a comment. @rsmith I see. Thank you for the clarification! Repository: rC Clang https://reviews.llvm.org/D53860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
rsmith added a comment. In https://reviews.llvm.org/D53860#1280959, @twoh wrote: > For me it seems that the bug is checking destructor accessibility of Base > itself, not fields of Base. The code is correct (or at least, following the current standard rule) as-is. The base class subobject itself is an element of the aggregate. Note that aggregate-initialization of `Derived` does not require `Base` to be an aggregate at all, so recursing to the fields of `Base` would be inappropriate. If this is causing significant problems for real code, we can certainly try to take this back to the C++ committee and request a do-over, but I think it's unlikely the rule would be changed: for aggregate initialization, direct base class subobjects are (by design) treated exactly the same as fields, and the destructors for all such subobjects are potentially invoked by aggregate initialization (because if an exception is thrown after an element is initialized and before initialization of the complete object finishes, the element's destructor will be invoked). You could argue that the context for the access check should be the derived class and not the context in which aggregate initialization is performed. I've checked the minutes, and we did indeed discuss that when resolving C++ DR 2227, and decided that the context for the access check should be the context of the aggregate initialization, not the context of the class definition. For what it's worth, this is not the only way that the C++17 extensions to aggregate initialization break existing code. There are easy workarounds: make sure that your class is not an aggregate (add a constructor), or don't use list-initialization. Repository: rC Clang https://reviews.llvm.org/D53860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
twoh added a comment.
I think the context is Derived here. My understanding of
http://wg21.link/p0968r0#2227 (in this patch's context) is that when Derived is
aggregate initialized, the destructor for each element of Base is potentially
invoked as well.
For me it seems that the bug is checking destructor accessibility of Base
itself, not fields of Base. I think the right fix for 1956-1960 is
if (!VerifyOnly) {
auto *BaseRD = Base.getType()->getAs()->getDecl();
for (FieldDecl *FD : BaseRD->fields()) {
QualType ET = SemaRef.Context.getBaseElementType(FD->getType());
if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
hadError = true;
return;
}
}
}
Repository:
rC Clang
https://reviews.llvm.org/D53860
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
ahatanak added a comment. http://wg21.link/p0968r0#2227 says: The destructor for each element of class type is potentially invoked (15.4 [class.dtor]) from the context where the aggregate initialization occurs. [Note: This provision ensures that destructors can be called for fully-constructed subobjects in case an exception is thrown (18.2 [except.ctor]). —end note] And '15.4 Destructors' has this sentence: A program is ill-formed if a destructor that is potentially invoked is deleted or not accessible from the context of the invocation. I'm not sure what "context" is in the case above (foo or Derived?), but if it's foo, it seems like clang is correct. Repository: rC Clang https://reviews.llvm.org/D53860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
rjmccall added a comment. That's interesting. If you think of a list-initialization of an aggregate as effectively defining an *ad hoc* constructor for it, then yes, we clearly ought to have access to protected destructors of base classes. And that aligns with the intuition that people make their destructors protected in order to prevent types from being constructed except as base sub-objects, which is still valid here. But at the same time, at a low level, we are directly accessing a protected destructor from a context that is not code actually defined in a subclass. Repository: rC Clang https://reviews.llvm.org/D53860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
modocache added a comment. Here's a compiler explorer link demonstrating that GCC 8.2 and Clang 7.0.0 compile the example code, but Clang trunk emits an error: https://godbolt.org/z/l3baI_ I realize the proposed "fix" here isn't likely to be exactly correct, but I thought it could be a good starting point. I'd also like to confirm that my example is, in fact, valid C++ code. Any and all reviews appreciated! Repository: rC Clang https://reviews.llvm.org/D53860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
modocache created this revision.
modocache added reviewers: rsmith, rjmccall, doug.gregor, ahatanak.
In https://reviews.llvm.org/D45898?id=157373, @rsmith advises to move a
`checkMemberDestructor` into `InitListChecker::CheckStructUnionTypes`,
to check base classes for an accessible destructor when performing
aggregate initialization. However, in so doing, the following C++ code
now fails to compile:
class Base { protected: ~Base() = default; };
struct Derived : Base {};
void foo() { Derived d = {}; }
GCC 8.2 and Clang 7.0.0 both treat this C++ code as valid, whereas
Clang trunk emits the following error diagnostic:
:3:27: error: temporary of type 'Base' has protected destructor
void foo() { Derived d = {}; }
^
:1:25: note: declared protected here
class Base { protected: ~Base() = default; };
^
I think Clang trunk is wrong about the validity of this C++ code: my
understanding of the C++17 standard is that the `Base` destructor need
only be accessible from within the context of `Derived`, and not at the
context in which `Derived` is aggregate initialized within the `foo`
function.
Add a test for the case above, and modify the destructor access check
within `InitListChecker::CheckStructUnionTypes` to check only that the
derived class has an accessible destructor.
Test Plan: `check-clang`
Repository:
rC Clang
https://reviews.llvm.org/D53860
Files:
lib/Sema/SemaInit.cpp
test/SemaCXX/aggregate-initialization.cpp
Index: test/SemaCXX/aggregate-initialization.cpp
===
--- test/SemaCXX/aggregate-initialization.cpp
+++ test/SemaCXX/aggregate-initialization.cpp
@@ -219,7 +219,7 @@
struct S0 { int f; ~S0() = delete; }; // expected-note {{'~S0' has been
explicitly marked deleted here}}
// Check destructor of base class.
-struct S3 : S0 {};
+struct S3 : S0 {}; // expected-note {{destructor of 'S3' is implicitly
deleted because base class 'ElementDestructor::BaseDestructor::S0' has a
deleted destructor}}
void test3() {
S3 s3 = {1}; // expected-error {{attempt to use a deleted function}}
@@ -233,4 +233,10 @@
struct B { B(); A a; };
struct C { B b; };
C c = { B() };
+
+ // Zylong's destructor doesn't have to be accessible from the context of
+ // Yerbl's initialization.
+ struct Zylong { protected: ~Zylong(); };
+ struct Yerbl : Zylong {};
+ Yerbl y = {};
}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -1954,7 +1954,7 @@
}
if (!VerifyOnly)
- if (hasAccessibleDestructor(Base.getType(), InitLoc, SemaRef)) {
+ if (hasAccessibleDestructor(DeclType, InitLoc, SemaRef)) {
hadError = true;
return;
}
Index: test/SemaCXX/aggregate-initialization.cpp
===
--- test/SemaCXX/aggregate-initialization.cpp
+++ test/SemaCXX/aggregate-initialization.cpp
@@ -219,7 +219,7 @@
struct S0 { int f; ~S0() = delete; }; // expected-note {{'~S0' has been explicitly marked deleted here}}
// Check destructor of base class.
-struct S3 : S0 {};
+struct S3 : S0 {}; // expected-note {{destructor of 'S3' is implicitly deleted because base class 'ElementDestructor::BaseDestructor::S0' has a deleted destructor}}
void test3() {
S3 s3 = {1}; // expected-error {{attempt to use a deleted function}}
@@ -233,4 +233,10 @@
struct B { B(); A a; };
struct C { B b; };
C c = { B() };
+
+ // Zylong's destructor doesn't have to be accessible from the context of
+ // Yerbl's initialization.
+ struct Zylong { protected: ~Zylong(); };
+ struct Yerbl : Zylong {};
+ Yerbl y = {};
}
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -1954,7 +1954,7 @@
}
if (!VerifyOnly)
- if (hasAccessibleDestructor(Base.getType(), InitLoc, SemaRef)) {
+ if (hasAccessibleDestructor(DeclType, InitLoc, SemaRef)) {
hadError = true;
return;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
