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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added inline comments.


================
Comment at: clang/include/clang/AST/DeclBase.h:1689
+    /// (used during overload resolution).
+    uint64_t DeductionCandidateKind : 2;
 
----------------
aaron.ballman wrote:
> Best not to give this the same name as a type (I don't care which one changes names).
I've changed the type name and kept this as is.


================
Comment at: clang/include/clang/Sema/Sema.h:3992
+      OverloadCandidateParamOrder PO = {},
+      bool AggregateCandidateDeduction = false);
   void AddFunctionCandidates(const UnresolvedSetImpl &Functions,
----------------
aaron.ballman wrote:
> We're up to 12 parameters for this function, five of which are `bool` parameters... at some point, this probably needs to be refactored.
Agreed. I will keep my eye on it.


================
Comment at: clang/lib/Sema/SemaInit.cpp:504-510
   InitListChecker(Sema &S, const InitializedEntity &Entity, InitListExpr *IL,
                   QualType &T, bool VerifyOnly, bool TreatUnavailableAsInvalid,
-                  bool InOverloadResolution = false);
+                  bool InOverloadResolution = false,
+                  SmallVector<QualType, 8> *AggrDeductionCandidateParamTypes = nullptr);
+  InitListChecker(Sema &S, const InitializedEntity &Entity, InitListExpr *IL,
+                  QualType &T,
+                  SmallVector<QualType, 8> &AggrDeductionCandidateParamTypes)
----------------
aaron.ballman wrote:
> We shouldn't force the caller to use the same-sized SmallVector, right?
That's right.


================
Comment at: clang/lib/Sema/SemaInit.cpp:1036
 
+RecordDecl *InitListChecker::getRecordDecl(QualType DeclType) {
+  if (DeclType->isRecordType())
----------------
aaron.ballman wrote:
> Can we make this return a `const RecordDecl *` or does that run into viral const issues?
"viral const issues". Deep somewhere else needs it non-const.


================
Comment at: clang/lib/Sema/SemaInit.cpp:1445-1447
+      //   brace elision is not considered for any aggregate element that has a
+      //   dependent non-array type or an array type with a value-dependent
+      //   bound
----------------
aaron.ballman wrote:
> Be sure to add test coverage for use of VLAs in C++ (we support it as an extension).
`Array::a2` test case covers this.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:2576
+    SourceLocation Loc) {
+  if (CXXRecordDecl *DefRecord =
+          cast<CXXRecordDecl>(Template->getTemplatedDecl())->getDefinition()) {
----------------
aaron.ballman wrote:
> Something is amiss here. Either this should be using `dyn_cast` or it should not be in an `if` statement (`cast` cannot fail; it asserts if it does).
It's the `getDefinition()` that may be null. I've hoist the cast out to make it obvious.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1059-1062
+    case CodeSynthesisContext::BuildingDeductionGuides:
+      assert(
+          false &&
+          "failed building deduction guides, add meaningful diagnostics here");
----------------
aaron.ballman wrote:
> cor3ntin wrote:
> > This seems unfinished
> +1
I meant to keep this a future work since this path is dead until some errors could be thrown out of this context.  In the future, if errors happen during building deduction guides, this assertion failure could trigger at build time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139837