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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

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



================
Comment at: clang/include/clang/AST/DeclCXX.h:1911
       setRangeEnd(EndLocation);
-    setIsCopyDeductionCandidate(false);
+    setDeductionCandidateKind(DeductionCandidateKind::Normal);
   }
----------------
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.


================
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;
   }
----------------
I'm not sure how useful these things are, isAggregateDeductionCandidate is only used once


================
Comment at: clang/lib/Sema/SemaInit.cpp:39
 #include "llvm/Support/raw_ostream.h"
+#include <optional>
 
----------------
This does not seems used


================
Comment at: clang/lib/Sema/SemaInit.cpp:312
   NoInitExpr *DummyExpr = nullptr;
+  SmallVector<QualType, 8> *AggrDeductionCandidateParamTypes = nullptr;
 
----------------
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?


================
Comment at: clang/lib/Sema/SemaInit.cpp:1098-1099
     maxElements = T->castAs<VectorType>()->getNumElements();
+  else if (T->isDependentType())
+    maxElements = 1;
   else
----------------
Can you had a comment about that case? I'm not sure i understand what scenario we are handling


================
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;
----------------
Maybe using `enumerate` + a range for loop here would be cleaner?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:7220
           MethodTmpl, ExplicitTemplateArgs, Args, Specialization, Info,
-          PartialOverloading, [&](ArrayRef<QualType> ParamTypes) {
+          PartialOverloading, false, [&](ArrayRef<QualType> ParamTypes) {
             return CheckNonDependentConversions(
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139837