ychen added a comment.
In D139837#3989814 <https://reviews.llvm.org/D139837#3989814>, @cor3ntin wrote:
> 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
Thanks for the quick fist-pass review. Appreciate it.
================
Comment at: clang/include/clang/AST/DeclCXX.h:1911
setRangeEnd(EndLocation);
- setIsCopyDeductionCandidate(false);
+ setDeductionCandidateKind(DeductionCandidateKind::Normal);
}
----------------
cor3ntin wrote:
> 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.
That's true indeed. The awkward aspect is that the
`CXXDeductionGuideDecl::Create` call is far from `setDeductionCandidateKind`;
making `CXXDeductionGuideDecl::Create` takes a `DeductionCandidateKind` would
several other less related functions takes `DeductionCandidateKind` also.
================
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;
}
----------------
cor3ntin wrote:
> I'm not sure how useful these things are, isAggregateDeductionCandidate is
> only used once
I meant to make it consistent with `isCopyDeductionCandidate` which is also
used once. Maybe remove both `isCopyDeductionCandidate` and
`isAggregateDeductionCandidate`?
================
Comment at: clang/lib/Sema/SemaInit.cpp:312
NoInitExpr *DummyExpr = nullptr;
+ SmallVector<QualType, 8> *AggrDeductionCandidateParamTypes = nullptr;
----------------
cor3ntin wrote:
> 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?
I meant to make `AggrDeductionCandidateParamTypes` a byref semantic like
`FullyStructuredList`. So the `InitListChecker` populates them as a
side-effect. Using optional means the `InitListChecker` owes the deduced types
which should work too but seems weird to me.
================
Comment at: clang/lib/Sema/SemaInit.cpp:1098-1099
maxElements = T->castAs<VectorType>()->getNumElements();
+ else if (T->isDependentType())
+ maxElements = 1;
else
----------------
cor3ntin wrote:
> Can you had a comment about that case? I'm not sure i understand what
> scenario we are handling
Good point. On second thought, I think this is not needed. This function
shouldn't accept dependent types.
================
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;
----------------
cor3ntin wrote:
> Maybe using `enumerate` + a range for loop here would be cleaner?
Using the iterator to test for the last element is easier. The index produced
by `enumerate` would need to produce an iterator nevertheless.
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
