rsmith added inline comments.
================
Comment at: clang/lib/AST/DeclCXX.cpp:1315-1316
!FieldRec->hasConstexprDefaultConstructor() && !isUnion())
// The standard requires any in-class initializer to be a constant
// expression. We consider this to be a defect.
+ data().DefaultedDefaultConstructorIsConstexpr =
----------------
Incidentally: this was fixed in DR1361, a long time ago. We can remove this
comment.
================
Comment at: clang/lib/AST/DeclCXX.cpp:1319
+ isa<ClassTemplateSpecializationDecl>(this)
+ ? hasConstexprDefaultConstructor()
+ : false;
----------------
Shouldn't this be looking in the class template itself? It doesn't seem correct
to use things like `this->hasConstexprDefaultConstructor()` from here, because
we've not finished gathering the data that contributes to that function's
return value yet.
That said... I don't think that changing
`DefaultedDefaultConstructorIsConstexpr` can work here. Keep in mind that a
class can have more than one default constructor, and in C++20 onwards can even
have more than one defaulted default constructor. This flag needs to apply to
all of them, and only some subset of them might have been declared as
`constexpr` in the template from which they are instantiated. So this flag
should be answering the question, "is a defaulted default constructor
*implicitly* constexpr?", and any callers that are assuming that it means "will
this particular defaulted default constructor be constexpr?" should be changed
to take into account whether the default constructor is explicitly declared as
constexpr.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:7555-7556
+ // expression.
+ if (!Constexpr && isa<ClassTemplateSpecializationDecl>(RD))
+ Constexpr = MD->isConstexpr();
+
----------------
It would be clearer and more in line with the wording to ask if `MD` is
instantiated here, rather than to ask if the class is being instantiated
(though the two are presumably equivalent because you can't instantiate a
constructor template to form a special member function).
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:7576
// would be.
MD->setConstexprKind(Constexpr ? (MD->isConsteval()
? ConstexprSpecKind::Consteval
----------------
I think we'll now treat instantiated constructors and non-instantiated ones
differently here:
```
#ifdef TEMPLATE
template<typename T>
#endif
struct S {
constexpr S() = default;
TypeWithThrowingConstructor x;
};
```
In the templated case, the instantiation's constructor will be constexpr, but
in the non-template case it won't be. That doesn't seem consistent. It would be
cleaner and would probably require fewer special cases if we treated both cases
the same: if you specify `constexpr`, you get a constexpr function, always, but
we'll generate an error message if the function is non-templated and wouldn't
have been implicitly constexpr.
================
Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:774
+ T data;
+ consteval default_ctor() = default; // expected-note {{non-constexpr
constructor 'foo' cannot be used in a constant expression}}
+};
----------------
This diagnostic demonstrates that we're not doing the proper checking here: we
do attempt to call a constexpr constructor that doesn't satisfy the constexpr
requirements, even though we're not supposed to. IIRC we try to hand-wave our
way to conformance here by saying that all of the things that cause a constexpr
constructor to fail to satisfy the constexpr requirements would also cause
evaluation to fail, but it'd be worth double-checking that (in particular: what
happens if the class has a virtual base class?).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128083/new/
https://reviews.llvm.org/D128083
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
