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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits