[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Sorry for not getting back to you earlier.
It mostly looks reasonable to me.
what do you think @erichkeane ?



================
Comment at: clang/lib/Sema/SemaInit.cpp:10338
+                    Context.getRValueReferenceType(ElementTypes[i]);
+              else if (isa<StringLiteral>(ListInit->getInit(i)))
+                // This deviates from the wording which is incorrect.
----------------
I think we need to support `A{("Hello")};` so we probably need to get rid of parentheses (maybe other implicit nodes?)


================
Comment at: clang/lib/Sema/SemaInit.cpp:10364
+            FunctionTemplateDecl *TD = GD->getDescribedFunctionTemplate();
+            assert(TD);
+            addDeductionCandidate(TD, GD, DeclAccessPair::make(TD, AS_public),
----------------
Maybe add a message here


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:2590-2591
+  // constructors into deduction guides.
+  // FIXME: Add a kind for this to give more meaningful diagnostics. But can
+  // this substitution process actually fail?
+  InstantiatingTemplate BuildingDeductionGuides(*this, Loc, Template);
----------------
Maybe we should have an asserton !isInvalid then?


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:368
       NewPack.push_back(Merged);
+        ++YA;
+      } else {
----------------
I think you can do that in the for loop as it was before


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:998
   bool IsPartiallyExpanded = false;
+  bool DeducePackIfNotAlreadyDeduced;
   /// The number of expansions, if we have a fully-expanded pack in this scope.
----------------
That could always be initialized, to avoid future surprises


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139837