Page MenuHomePhabricator

[Clang] Implements CTAD for aggregates P1816R0 and P2082R1
Needs ReviewPublic

Authored by ychen on Dec 12 2022, 5:22 AM.

Details

Reviewers
aaron.ballman
erichkeane
rsmith
shafik
cor3ntin
Group Reviewers
Restricted Project
Summary

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

Diff Detail

Event Timeline

ychen created this revision.Dec 12 2022, 5:22 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: martong. · View Herald Transcript
ychen requested review of this revision.Dec 12 2022, 5:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 5:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ychen added a reviewer: Restricted Project.Dec 12 2022, 5:23 AM

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 updated this revision to Diff 482331.Dec 12 2022, 6:42 PM
ychen marked 2 inline comments as done.
  • Address Corentin's comments
ychen added a comment.Dec 12 2022, 6:42 PM

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 updated this revision to Diff 483735.Dec 17 2022, 12:53 AM
  • update cxx_status.html

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 added inline comments.Jan 5 2023, 4:30 AM
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?)

cor3ntin added inline comments.Jan 6 2023, 4:20 AM
clang/lib/Sema/SemaInit.cpp
13

Do we actually need that?

shafik added inline comments.Jan 12 2023, 11:25 AM
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 added a subscriber: ayzhao.Jan 19 2023, 3:15 PM

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 added a comment.Jan 23 2023, 6:21 PM

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.

ecatmur added a subscriber: ecatmur.Mon, Mar 6, 7:00 PM