except that parenthesized expression-list support is left as TODO since
it depends on D129531.
Details
- Reviewers
aaron.ballman erichkeane rsmith shafik cor3ntin - Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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
clang/include/clang/AST/DeclCXX.h | ||
---|---|---|
1911 | 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. | |
1953–1959 | I'm not sure how useful these things are, isAggregateDeductionCandidate is only used once | |
clang/lib/Sema/SemaInit.cpp | ||
39 | This does not seems used | |
311 | I think using optional here would make more sense, I guess that's why you included it. | |
1097–1098 | Can you had a comment about that case? I'm not sure i understand what scenario we are handling | |
2164–2166 | Maybe using enumerate + a range for loop here would be cleaner? | |
clang/lib/Sema/SemaOverload.cpp | ||
7220–7221 |
Thanks for the quick fist-pass review. Appreciate it.
clang/include/clang/AST/DeclCXX.h | ||
---|---|---|
1911 | 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. | |
1953–1959 | I meant to make it consistent with isCopyDeductionCandidate which is also used once. Maybe remove both isCopyDeductionCandidate and isAggregateDeductionCandidate? | |
clang/lib/Sema/SemaInit.cpp | ||
311 | 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. | |
1097–1098 | Good point. On second thought, I think this is not needed. This function shouldn't accept dependent types. | |
2164–2166 | Using the iterator to test for the last element is easier. The index produced by enumerate would need to produce an iterator nevertheless. |
Sorry for not getting back to you earlier.
It mostly looks reasonable to me.
what do you think @erichkeane ?
clang/lib/Sema/SemaInit.cpp | ||
---|---|---|
10338 | I think we need to support A{("Hello")}; so we probably need to get rid of parentheses (maybe other implicit nodes?) | |
10364 | Maybe add a message here | |
clang/lib/Sema/SemaTemplate.cpp | ||
2590–2591 | Maybe we should have an asserton !isInvalid then? | |
clang/lib/Sema/SemaTemplateDeduction.cpp | ||
368 | I think you can do that in the for loop as it was before | |
998 | That could always be initialized, to avoid future surprises |
clang/include/clang/AST/DeclCXX.h | ||
---|---|---|
1953–1959 | Yes, i think we might as well |
+ @hubert.reinterpretcast In case i missed something conformance wise.
Speaking of conformance, your implementation does not appear to gate the feature on C++20, which it should, so you should add a test that it doesn't compile in c++17 (I'm not sure we could support it as an extension... maybe?)
clang/lib/Sema/SemaInit.cpp | ||
---|---|---|
13 | Do we actually need that? |
clang/lib/Sema/SemaInit.cpp | ||
---|---|---|
10277–10281 | Minor issues, we need to match the format expected by bugprone-comment | |
clang/lib/Sema/SemaOverload.cpp | ||
7341 | formatting fix again. | |
clang/lib/Sema/SemaTemplate.cpp | ||
2587 | formatting. |
Thanks for your effort on this!
FYI I landed parenthesized aggregate initialization in D141546, so CTAD for that feature should no longer be blocked.
Thanks for letting me know. Considering the existing reviews, I'll create a separate patch for the parenthesized expression-list support.