[PATCH] D128083: [C++20] Correctly handle constexpr/consteval explicitly defaulted special member instantiation behavior

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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