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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D139837#3989814 <https://reviews.llvm.org/D139837#3989814>, @cor3ntin wrote:

> Hey. Thanks a lot for working on this.
> I did a first pass, looking at mostly style issues, looking at conformance will probably take me a lot more time, but i think this looks pretty good overall

Thanks for the quick fist-pass review. Appreciate it.



================
Comment at: clang/include/clang/AST/DeclCXX.h:1911
       setRangeEnd(EndLocation);
-    setIsCopyDeductionCandidate(false);
+    setDeductionCandidateKind(DeductionCandidateKind::Normal);
   }
----------------
cor3ntin wrote:
> I'm wondering if the constructor should take a `DeductionCandidateKind` defaulted to normal here. All the places where it's set seem to be immediately after construction.
That's true indeed. The awkward aspect is that the `CXXDeductionGuideDecl::Create` call is far from `setDeductionCandidateKind`; making `CXXDeductionGuideDecl::Create` takes a `DeductionCandidateKind` would several other less related functions takes `DeductionCandidateKind` also.


================
Comment at: clang/include/clang/AST/DeclCXX.h:1953-1959
   bool isCopyDeductionCandidate() const {
-    return FunctionDeclBits.IsCopyDeductionCandidate;
+    return getDeductionCandidateKind() == DeductionCandidateKind::Copy;
+  }
+
+  bool isAggregateDeductionCandidate() const {
+    return getDeductionCandidateKind() == DeductionCandidateKind::Aggregate;
   }
----------------
cor3ntin wrote:
> I'm not sure how useful these things are, isAggregateDeductionCandidate is only used once
I meant to make it consistent with `isCopyDeductionCandidate` which is also used once. Maybe remove both `isCopyDeductionCandidate` and `isAggregateDeductionCandidate`?


================
Comment at: clang/lib/Sema/SemaInit.cpp:312
   NoInitExpr *DummyExpr = nullptr;
+  SmallVector<QualType, 8> *AggrDeductionCandidateParamTypes = nullptr;
 
----------------
cor3ntin wrote:
> I think using optional here would make more sense, I guess that's why you included it.
> Should it be SmallVectorImpl, such that only the caller has to bake in a size?
I meant to make `AggrDeductionCandidateParamTypes` a byref semantic like `FullyStructuredList`. So the  `InitListChecker` populates them as a side-effect. Using optional means the `InitListChecker` owes the deduced types which should work too but seems weird to me. 


================
Comment at: clang/lib/Sema/SemaInit.cpp:1098-1099
     maxElements = T->castAs<VectorType>()->getNumElements();
+  else if (T->isDependentType())
+    maxElements = 1;
   else
----------------
cor3ntin wrote:
> Can you had a comment about that case? I'm not sure i understand what scenario we are handling
Good point. On second thought, I think this is not needed. This function shouldn't accept dependent types.


================
Comment at: clang/lib/Sema/SemaInit.cpp:2167-2168
   // If we have any base classes, they are initialized prior to the fields.
-  for (auto &Base : Bases) {
+  for (auto I = Bases.begin(), E = Bases.end(); I != E; ++I) {
+    auto &Base = *I;
     Expr *Init = Index < IList->getNumInits() ? IList->getInit(Index) : nullptr;
----------------
cor3ntin wrote:
> Maybe using `enumerate` + a range for loop here would be cleaner?
Using the iterator to test for the last element is easier. The index produced by `enumerate` would need to produce an iterator nevertheless.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139837