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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this! Some high-level comments while I was here; I'm still grokking the implementation.



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


================
Comment at: clang/include/clang/AST/DeclCXX.h:1987
+  void setDeductionCandidateKind(DeductionCandidateKind K) {
+    FunctionDeclBits.DeductionCandidateKind = static_cast<uint64_t>(K);
   }
----------------
Er, seems a bit odd to cast an 8-bit type to a 64-bit type only to shove it into a 2-bit bit-field. I think `DeductionCandidateKind` should be an enum class whose underlying type is `int` and we cast to/from `int` as needed.


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


================
Comment at: clang/include/clang/Sema/Sema.h:9346
+      /// We are building deduction guides for a class.
+      BuildingDeductionGuides
     } Kind;
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:9661
+    struct BuildingDeductionGuides {};
+    /// Note that we are instantiating an exception specification
+    /// of a function template.
----------------
cor3ntin wrote:
> Is that comment correct?
Yeah, the comment seems off to me.


================
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)
----------------
We shouldn't force the caller to use the same-sized SmallVector, right?


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


================
Comment at: clang/lib/Sema/SemaInit.cpp:1037-1038
+RecordDecl *InitListChecker::getRecordDecl(QualType DeclType) {
+  if (DeclType->isRecordType())
+    return DeclType->castAs<RecordType>()->getDecl();
+  if (auto *Inject = dyn_cast<InjectedClassNameType>(DeclType))
----------------



================
Comment at: clang/lib/Sema/SemaInit.cpp:1039-1040
+    return DeclType->castAs<RecordType>()->getDecl();
+  if (auto *Inject = dyn_cast<InjectedClassNameType>(DeclType))
+    return Inject->getDecl();
+  return nullptr;
----------------



================
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
----------------
Be sure to add test coverage for use of VLAs in C++ (we support it as an extension).


================
Comment at: clang/lib/Sema/SemaInit.cpp:10711-10712
+        //   otherwise, T_i is the declared type of e_i
+        for (int i = 0, e = ListInit->getNumInits();
+             i < e && !isa<PackExpansionType>(ElementTypes[i]); ++i)
+          if (ElementTypes[i]->isArrayType()) {
----------------



================
Comment at: clang/lib/Sema/SemaInit.cpp:10754
       auto *TD = dyn_cast<FunctionTemplateDecl>(D);
       auto *GD = dyn_cast_or_null<CXXDeductionGuideDecl>(
           TD ? TD->getTemplatedDecl() : dyn_cast<FunctionDecl>(D));
----------------



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:2576
+    SourceLocation Loc) {
+  if (CXXRecordDecl *DefRecord =
+          cast<CXXRecordDecl>(Template->getTemplatedDecl())->getDefinition()) {
----------------
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).


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1059-1062
+    case CodeSynthesisContext::BuildingDeductionGuides:
+      assert(
+          false &&
+          "failed building deduction guides, add meaningful diagnostics here");
----------------
cor3ntin wrote:
> This seems unfinished
+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139837