[Clang] Implements CTAD for aggregates P1816R0 and P2082R1

[ychen]

Summary

except that parenthesized expression-list support is left as TODO since
it depends on D129531.

[ychen]

format

[cor3ntin]

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.
Should it be SmallVectorImpl, such that only the caller has to bake in a size?

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

[ychen]

• Address Corentin's comments

[ychen]

In 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.

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.

[ychen]

• update cxx_status.html

[cor3ntin]

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

[cor3ntin]

clang/include/clang/AST/DeclCXX.h

1953–1959

Yes, i think we might as well

[cor3ntin]

+ @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?)

[cor3ntin]

clang/lib/Sema/SemaInit.cpp

13

Do we actually need that?

[shafik]

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.

[ayzhao]

Thanks for your effort on this!

FYI I landed parenthesized aggregate initialization in D141546, so CTAD for that feature should no longer be blocked.

[ychen]

In D139837#4067208, @ayzhao wrote:

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.