This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Implements CTAD for aggregates P1816R0 and P2082R1
ClosedPublic

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

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
1951

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.

1989–1993

I'm not sure how useful these things are, isAggregateDeductionCandidate is only used once

clang/lib/Sema/SemaInit.cpp
35

This does not seems used

310

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?

1101–1102

Can you had a comment about that case? I'm not sure i understand what scenario we are handling

2185–2187

Maybe using enumerate + a range for loop here would be cleaner?

clang/lib/Sema/SemaOverload.cpp
7265–7266
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
1951

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.

1989–1993

I meant to make it consistent with isCopyDeductionCandidate which is also used once. Maybe remove both isCopyDeductionCandidate and isAggregateDeductionCandidate?

clang/lib/Sema/SemaInit.cpp
310

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.

1101–1102

Good point. On second thought, I think this is not needed. This function shouldn't accept dependent types.

2185–2187

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
10805

Maybe add a message here

10815

I think we need to support A{("Hello")}; so we probably need to get rid of parentheses (maybe other implicit nodes?)

clang/lib/Sema/SemaTemplate.cpp
2596–2597

Maybe we should have an asserton !isInvalid then?

clang/lib/Sema/SemaTemplateDeduction.cpp
363

I think you can do that in the for loop as it was before

995

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
1989–1993

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
10674–10678

Minor issues, we need to match the format expected by bugprone-comment

clang/lib/Sema/SemaOverload.cpp
7386–7387

formatting fix again.

clang/lib/Sema/SemaTemplate.cpp
2593

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.Mar 6 2023, 7:00 PM

@ychen Hey! Are you still working on this? Thanks!

@erichkeane @aaron.ballman I think we should determine how much works remains there and if the author is not responsive maybe one of us can push this up the finishing line.

@erichkeane @aaron.ballman I think we should determine how much works remains there and if the author is not responsive maybe one of us can push this up the finishing line.

That seems like a reasonable approach to me; it'd be good to get this into Clang 17 if possible, and @ychen seems to have been away for ~2 months now, so it seems reasonable to commandeer this review if we need to.

ychen added a comment.May 11 2023, 6:36 PM

Sorry for the long delay guys. I'll update the patch this weekend.

ychen marked 9 inline comments as done.May 13 2023, 9:40 PM
ychen added inline comments.
clang/lib/Sema/SemaInit.cpp
10815

Agreed. Fixed.

clang/lib/Sema/SemaTemplate.cpp
2596–2597

Looked at this a little bit more. It can fail (D46446). So I updated the comments in a few places.

clang/lib/Sema/SemaTemplateDeduction.cpp
363

Done

ychen updated this revision to Diff 521963.May 13 2023, 9:41 PM
ychen marked 3 inline comments as done.

Address all comments

ychen updated this revision to Diff 521965.May 13 2023, 9:52 PM
ychen marked an inline comment as done.
  • add release notes

I think this is starting to look good, My biggest question is the remaining fixme, how much work would it be to do in this PR?

clang/include/clang/Sema/TemplateDeduction.h
237

Can you specify which c++ version this quote is from?

clang/lib/Sema/SemaTemplate.cpp
2597

Maybe you could do this in this PR?

clang/www/cxx_status.html
1249 ↗(On Diff #521965)

I think this should be marked a partially supported until we support parenthesized init.

ychen updated this revision to Diff 521978.May 14 2023, 12:39 AM
  • Fix bitfield on Windows
ychen marked 3 inline comments as done.May 20 2023, 10:46 AM
ychen updated this revision to Diff 524045.EditedMay 20 2023, 10:47 AM

@cor3ntin Thanks for the review. I've added the parenthesized expression-list support by treating it as braced-init-list during CTAD.

  • adjust test check for Windows
  • support parenthesized expression-list
  • add an InitializationKind BuildingDeductionGuides to help diagnoses

Thanks, It's great to have the complete feature in on PR

clang/include/clang/Sema/Sema.h
9684

Is that comment correct?

clang/lib/Sema/SemaDecl.cpp
12703–12705
clang/lib/Sema/SemaTemplateInstantiate.cpp
1062–1065

This seems unfinished

Thank you for working on this! Some high-level comments while I was here; I'm still grokking the implementation.

clang/include/clang/AST/DeclBase.h
1691

Best not to give this the same name as a type (I don't care which one changes names).

clang/include/clang/AST/DeclCXX.h
1986

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.

clang/include/clang/Sema/Sema.h
4000

We're up to 12 parameters for this function, five of which are bool parameters... at some point, this probably needs to be refactored.

9369
aaron.ballman added inline comments.May 23 2023, 9:43 AM
clang/include/clang/Sema/Sema.h
9684

Yeah, the comment seems off to me.

clang/lib/Sema/SemaInit.cpp
505–513

We shouldn't force the caller to use the same-sized SmallVector, right?

1034

Can we make this return a const RecordDecl * or does that run into viral const issues?

1035–1036
1037–1038
1443–1445

Be sure to add test coverage for use of VLAs in C++ (we support it as an extension).

10710–10711
10755
clang/lib/Sema/SemaTemplate.cpp
2576

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

clang/lib/Sema/SemaTemplateInstantiate.cpp
1062–1065

+1

ychen updated this revision to Diff 528268.Jun 4 2023, 8:42 PM
ychen marked 14 inline comments as done.
  • address existing comments
clang/include/clang/AST/DeclBase.h
1691

I've changed the type name and kept this as is.

clang/include/clang/Sema/Sema.h
4000

Agreed. I will keep my eye on it.

clang/lib/Sema/SemaInit.cpp
505–513

That's right.

1034

"viral const issues". Deep somewhere else needs it non-const.

1443–1445

Array::a2 test case covers this.

clang/lib/Sema/SemaTemplate.cpp
2576

It's the getDefinition() that may be null. I've hoist the cast out to make it obvious.

clang/lib/Sema/SemaTemplateInstantiate.cpp
1062–1065

I meant to keep this a future work since this path is dead until some errors could be thrown out of this context. In the future, if errors happen during building deduction guides, this assertion failure could trigger at build time.

cor3ntin added inline comments.Jun 4 2023, 10:52 PM
clang/lib/Sema/SemaTemplateInstantiate.cpp
1062–1065

In this case, maybe change the message
assert(false && "unexpected deduction guide in instantiation stack") or something along those lines.

ychen updated this revision to Diff 528918.Jun 6 2023, 10:06 AM
  • address comment
ychen added inline comments.Jun 6 2023, 10:07 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
1062–1065

Done. I've made it llvm_unreachable to catch the rare chances that users hit this.

I think this looks good but I'll let @aaron.ballman do the final approval.

ychen added a comment.Jun 6 2023, 5:58 PM

I think this looks good but I'll let @aaron.ballman do the final approval.

Thanks for the review @cor3ntin.

shafik added inline comments.Jun 8 2023, 9:15 PM
clang/test/SemaTemplate/aggregate-deduction-candidate.cpp
102

I would also like to see this test:

template <typename T>
struct I {
  using type = T;
};

template <typename T>
struct E {
  typename I<T>::type i;
  T t;
};

E e1 = {1, 2}; // OK, E<int> deduced

Since it is in the paper, this should cover it but it can't hurt.

ychen updated this revision to Diff 530057.Jun 9 2023, 12:49 PM
  • add one extra test
ychen marked an inline comment as done.Jun 9 2023, 12:50 PM
ychen added inline comments.
clang/test/SemaTemplate/aggregate-deduction-candidate.cpp
102

Yep, just added this test.

shafik added inline comments.Jun 9 2023, 5:31 PM
clang/include/clang/AST/DeclBase.h
1688–1691

Why remove [C++17] ?

clang/include/clang/AST/DeclCXX.h
1986

It feels a bit weird that we are taking an enum w/ an underlying type of unsigned char casting it to int and then placing it in an unsigned bit-field. I don't have a better suggestion ATM but I wish we had something better.

clang/lib/Sema/SemaDecl.cpp
12713

nitpick but we seem to use Expr* everywhere else.

clang/lib/Sema/SemaInit.cpp
15

I saw your adding headers. How did you figure out which ones were missing?

508

nit

10678

nit,

note: PO` is not a very descriptive name.

10734
10741
10763–10781

Quoted Text

cor3ntin added inline comments.Jun 14 2023, 1:33 AM
clang/include/clang/AST/DeclBase.h
1688–1691

I don't think this adds any information, I'm happy to see it removed. And arguably in c++17 there was only one kind of DG.

ychen updated this revision to Diff 533674.Jun 22 2023, 10:06 AM
ychen marked 7 inline comments as done.
  • address comments
clang/include/clang/AST/DeclCXX.h
1986

I've changed it to casting to underlying type of DeductionCandidate so it has one less step of conversion.

clang/lib/Sema/SemaInit.cpp
15

Added headers provide APIs for the new code. I guess many of them are included already indirectly (SmallVector for example). But I thought the best practice is not relying on that, so I added these.

cor3ntin added inline comments.Jun 25 2023, 12:18 AM
clang/lib/Sema/SemaInit.cpp
15

I think it's easier fore reviewers not to add things are are not needed, I would suggest trying to keep the change to the list of includes to the minimum

ychen updated this revision to Diff 534601.Jun 26 2023, 9:22 AM
  • remove extra headers

@ychen You probably want to update the description, i think it's updated (you implemented ctad for parenthesized ctrs)

ychen edited the summary of this revision. (Show Details)Jun 26 2023, 10:21 AM

@ychen You probably want to update the description, i think it's updated (you implemented ctad for parenthesized ctrs)

Thanks for the remainder, Corentin. Hopefully, this will be ready to be landed soon.

@ychen You probably want to update the description, i think it's updated (you implemented ctad for parenthesized ctrs)

Thanks for the remainder, Corentin. Hopefully, this will be ready to be landed soon.

I hope so too. I pinged @aaron.ballman :)

Thanks for this! I have no concerns from what I can see, but I see the others are doing a very thorough review, so I'll let them do the approvals.

clang/include/clang/AST/DeclCXX.h
1951

I'm in favor of having this be a part of CXXDeductionGuideDecl::Create and the ctor as a parameter instead as well.

ychen updated this revision to Diff 534778.Jun 26 2023, 4:15 PM
  • add a DeductionCandidate parameter
cor3ntin added inline comments.Jun 27 2023, 7:29 AM
clang/include/clang/AST/DeclBase.h
1689
clang/include/clang/AST/DeclCXX.h
1989–1993

I still think we should remove that

clang/include/clang/Sema/Sema.h
9683

Maybe rename that BuildingDeductionGuidesTag

ychen updated this revision to Diff 535132.Jun 27 2023, 2:15 PM
ychen marked 3 inline comments as done.
  • address comments
ychen marked an inline comment as done.Jun 27 2023, 2:16 PM
cor3ntin accepted this revision.Jun 27 2023, 11:50 PM

Please give @aaron.ballman a few days to get another look, but otherwise LGTM
Thanks a lot for working on this and your patience during the review.

This revision is now accepted and ready to land.Jun 27 2023, 11:50 PM
aaron.ballman accepted this revision.Jun 28 2023, 6:53 AM

LGTM aside from some minor issues, thank you!

clang/docs/ReleaseNotes.rst
113

Paren aggregate init is now supported, so this should be updated.

clang/lib/Sema/SemaInit.cpp
3271–3272

For local style consistency.

10620–10688
clang/lib/Sema/SemaTemplate.cpp
2577–2580

I think this is a bit more clear as to what's happening.

ychen updated this revision to Diff 535883.Jun 29 2023, 10:30 AM
ychen marked 4 inline comments as done.
  • address comments
clang/docs/ReleaseNotes.rst
113

Thanks for catching this.

Thanks for the review!

Thanks a lot for working on this!
Do you need Aaron or myself to commit this for you? If so, which name / mail do you want us to use?

Thanks a lot for working on this!
Do you need Aaron or myself to commit this for you? If so, which name / mail do you want us to use?

That's okay. I'm doing the rebase and I could commit it later.

ychen updated this revision to Diff 535936.Jun 29 2023, 11:33 AM
  • rebase
This revision was landed with ongoing or failed builds.Jun 29 2023, 2:23 PM
This revision was automatically updated to reflect the committed changes.

The changes to clang/www/cxx_status.html got lost during the rebase, can you fix that? Thanks!

So while working on D148474 I realized this PR introduced a new crash bug, see the following code: https://godbolt.org/z/h1EezGjbr

template<typename A3>
class B3 : A3 {
  template<bool = C3<B3>()>
  B3();
}; B3();

which is one of my test cases. I had though it was an interaction between my new code and this but then realized this exists w/o my new code. We can also see from the godbolt above that the crash in this code changed from an assertion to an unreachable. My PR will fix the assertion but I need a good solution to the unreachable bug. I tried changing the unreachable to a break but it reduces the quality of the diagnostic and I don't think it is the correct fix.