[libcxx-commits] [PATCH] D156032: Implement CWG2137 (list-initialization from objects of the same type)

[Aaron Ballman via Phabricator via libcxx-commits]

aaron.ballman added a subscriber: Endill.
aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaInit.cpp:4263-4264
+      // CWG2311: T{ prvalue_of_type_T } is not eligible for copy elision
+      // Make this an elision if this won't call an initializer-list constructor
+      // (Always on an aggregate type or check constructors first)
+      CopyElisionPossible = true;
----------------



================
Comment at: clang/lib/Sema/SemaInit.cpp:4334
+      case OR_No_Viable_Function: // Possible if you try to initialize from a
+                                  // volatile prvalue
+      case OR_Ambiguous:
----------------



================
Comment at: clang/lib/Sema/SemaInit.cpp:4337
+        // This was never going to be an initializer-list constructor
+        // so it should be elided
+        ElideConstructor();
----------------



================
Comment at: clang/lib/Sema/SemaInit.cpp:4342
+        // If this deleted constructor was not an initializer-list constructor
+        // we should elide it
+        if (!S.isInitListConstructor(Best->Function)) {
----------------



================
Comment at: clang/lib/Sema/SemaInit.cpp:4249-4250
           InitializedEntity::EK_LambdaToBlockConversionBlockElement &&
-      UnwrappedArgs.size() == 1 && UnwrappedArgs[0]->isPRValue() &&
-      S.Context.hasSameUnqualifiedType(UnwrappedArgs[0]->getType(), DestType)) {
+      Args.size() == 1 && Args[0]->isPRValue() &&
+      S.Context.hasSameUnqualifiedType(Args[0]->getType(), DestType)) {
     // Convert qualifications if necessary.
----------------
MitalAshok wrote:
> MitalAshok wrote:
> > This change unfortunately exposes the still-open [[ https://wg21.link/CWG2311 | CWG2311 ]] but allows `T{ object_of_type_T }` to consider user declared constructors.
> > 
> > I am working on a separate fix for CWG2311 (Consider constructors as below, but then if the chosen constructor is not an initializer-list constructor, elide it).
> > 
> Fix will be here: https://reviews.llvm.org/D156062
> 
> `auto{ prvalue }` will elide a copy for aggregate types again. It will do so after checking constructors for non-aggregate classes now.
As this was abandoned in favor of doing the fix here, the patch title, summary, and release notes should all be updated to talk about CWG2311.


================
Comment at: clang/test/CXX/drs/dr23xx.cpp:54
+void test() {
+  // Ensure none of these expressions try to call a move constructor
+  T a = T{T(0)};
----------------



================
Comment at: clang/test/CXX/drs/dr23xx.cpp:9
+namespace std {
+  __extension__ typedef __SIZE_TYPE__ size_t;
+
----------------
MitalAshok wrote:
> cor3ntin wrote:
> > 
> Wouldn't work in c++98 mode this file is being run in. I've copied this from other tests in this directory. I guess I can put a `#if __cplusplus >= 201103L` instead?
The current form seems fine to me.


================
Comment at: clang/test/CXX/drs/dr23xx.cpp:50
 
+namespace dr2311 {
+#if __cplusplus >= 201707L
----------------
MitalAshok wrote:
> cor3ntin wrote:
> > the dr status html page is generated automatically by leaving a magic comment here - and then running `clang/www/make_cxx_dr_status`
> CWG2311 is still open, but clang did support list-initialization elision before this patch. The committee still needs to decide how exactly this elision is going to work, which might be different from how this patch implements it, so hesitant to mark this as done.
CC @Endill -- do we have a special marking for "issue still open in WG21, but we implement the current suggested resolution?"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156032/new/

https://reviews.llvm.org/D156032