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
