[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

shafik added a comment. Note we have a second crash linked to this PR: https://github.com/llvm/llvm-project/issues/67173#issuecomment-1733647699 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

shafik added a comment. So while working on D148474 I realized this PR introduced a new crash bug, see the following code: https://godbolt.org/z/h1EezGjbr template class B3 : A3 { template()> B3(); }; B3(); which is one of my test

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. The changes to clang/www/cxx_status.html got lost during the rebase, can you fix that? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG632dd6a4ca00: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1 (authored by ychen). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 535936. ychen added a comment. - rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/DeclBase.h

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added a comment. In D139837#4460664 , @cor3ntin wrote: > 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

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added a comment. Thanks for the review! 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

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added inline comments. Comment at: clang/docs/ReleaseNotes.rst:98 +- Implemented `P1816R0: `_ and `P2082R1: `_, + which allows CTAD for aggregates (parenthesized aggregate-initialization is not supported).

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 535883. ychen marked 4 inline comments as done. ychen added a comment. - address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files:

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from some minor issues, thank you! Comment at: clang/docs/ReleaseNotes.rst:98 +- Implemented `P1816R0: `_ and `P2082R1: `_, + which allows

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin accepted this revision. cor3ntin added a comment. This revision is now accepted and ready to land. 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. Repository: rG LLVM Github Monorepo

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 535132. ychen marked 3 inline comments as done. ychen added a comment. - address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files:

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added inline comments. Comment at: clang/include/clang/AST/DeclBase.h:1687 +/// Only used by CXXDeductionGuideDecl. Indicates the kind +/// of the Deduction Guide that is the implicitly generated +/// (used during overload resolution).

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 534778. ychen added a comment. - add a DeductionCandidate parameter Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files: clang/docs/ReleaseNotes.rst

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

erichkeane added a comment. 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. Comment at: clang/include/clang/AST/DeclCXX.h:1911 setRangeEnd(EndLocation); -

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. In D139837#4449536 , @ychen wrote: > In D139837#4449327 , @cor3ntin > wrote: > >> @ychen You probably want to update the description, i think it's updated >> (you implemented ctad for

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added a comment. In D139837#4449327 , @cor3ntin wrote: > @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

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. @ychen You probably want to update the description, i think it's updated (you implemented ctad for parenthesized ctrs) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 534601. ychen added a comment. - remove extra headers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files: clang/docs/ReleaseNotes.rst

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:15 #include "clang/AST/DeclObjC.h" +#include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" ychen wrote: > shafik wrote: > > I saw your adding headers. How did you figure out which ones

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:1987 + void setDeductionCandidateKind(DeductionCandidateKind K) { +FunctionDeclBits.DeductionCandidateKind = static_cast(K); } shafik wrote: > aaron.ballman wrote: > > Er, seems a

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 533674. ychen marked 7 inline comments as done. ychen added a comment. - address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files:

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added inline comments. Comment at: clang/include/clang/AST/DeclBase.h:1686 -/// [C++17] Only used by CXXDeductionGuideDecl. Indicates that -/// the Deduction Guide is the implicitly generated 'copy -/// deduction candidate' (is used during overload

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

shafik added inline comments. Comment at: clang/include/clang/AST/DeclBase.h:1686 -/// [C++17] Only used by CXXDeductionGuideDecl. Indicates that -/// the Deduction Guide is the implicitly generated 'copy -/// deduction candidate' (is used during overload

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen marked an inline comment as done. ychen added inline comments. Comment at: clang/test/SemaTemplate/aggregate-deduction-candidate.cpp:101 + + template struct E { +T t; shafik wrote: > I would also like to see this test: > > ``` > template > struct I

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 530057. ychen added a comment. - add one extra test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files: clang/docs/ReleaseNotes.rst

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

shafik added inline comments. Comment at: clang/test/SemaTemplate/aggregate-deduction-candidate.cpp:101 + + template struct E { +T t; I would also like to see this test: ``` template struct I { using type = T; }; template struct E { typename

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added a comment. In D139837#4401134 , @cor3ntin wrote: > I think this looks good but I'll let @aaron.ballman do the final approval. Thanks for the review @cor3ntin. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. I think this looks good but I'll let @aaron.ballman do the final approval. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 ___

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1059-1062 +case CodeSynthesisContext::BuildingDeductionGuides: + assert( + false && + "failed building deduction guides, add meaningful diagnostics here");

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 528918. ychen added a comment. - address comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files: clang/docs/ReleaseNotes.rst

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1059-1062 +case CodeSynthesisContext::BuildingDeductionGuides: + assert( + false && + "failed building deduction guides, add meaningful diagnostics here");

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added inline comments. Comment at: clang/include/clang/AST/DeclBase.h:1689 +/// (used during overload resolution). +uint64_t DeductionCandidateKind : 2; aaron.ballman wrote: > Best not to give this the same name as a type (I don't care which one

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 528268. ychen marked 14 inline comments as done. ychen added a comment. - address existing comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files:

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

aaron.ballman added a comment. Thank you for working on this! Some high-level comments while I was here; I'm still grokking the implementation. Comment at: clang/include/clang/AST/DeclBase.h:1689 +/// (used during overload resolution). +uint64_t DeductionCandidateKind

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. Thanks, It's great to have the complete feature in on PR Comment at: clang/include/clang/Sema/Sema.h:9661 +struct BuildingDeductionGuides {}; +/// Note that we are instantiating an exception specification +/// of a function template.

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 524045. ychen added a comment. - fix - Address comments - fix - - adjust test check for Windows Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files:

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 521978. ychen added a comment. - Fix bitfield on Windows Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files: clang/docs/ReleaseNotes.rst

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. 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? Comment at: clang/include/clang/Sema/TemplateDeduction.h:237 + // C++ [over.match.class.deduct]p5.2: + // During

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 521965. ychen marked an inline comment as done. ychen added a comment. - add release notes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files:

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 521964. ychen added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files: clang/include/clang/AST/DeclBase.h clang/include/clang/AST/DeclCXX.h

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 521963. ychen marked 3 inline comments as done. ychen added a comment. Address all comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files:

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen marked 9 inline comments as done. ychen added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:10338 +Context.getRValueReferenceType(ElementTypes[i]); + else if (isa(ListInit->getInit(i))) +// This deviates from the

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added a comment. Sorry for the long delay guys. I'll update the patch this weekend. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 ___ cfe-commits

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

aaron.ballman added a comment. In D139837#4326501 , @cor3ntin wrote: > @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.

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. @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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. @ychen Hey! Are you still working on this? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 ___ cfe-commits mailing list

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added a comment. 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

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ayzhao added a comment. Thanks for your effort on this! FYI I landed parenthesized aggregate initialization in D141546 , so CTAD for that feature should no longer be blocked. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

shafik added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:10277-10281 + AddTemplateOverloadCandidate(TD, FoundDecl, /*ExplicitArgs*/ nullptr, + TmpInits, Candidates, + SuppressUserConversions, +

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:13 +#include "clang/AST/APValue.h" #include "clang/AST/ASTContext.h" Do we actually need that? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a subscriber: hubert.reinterpretcast. cor3ntin added a comment. + @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

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:1953-1959 bool isCopyDeductionCandidate() const { -return FunctionDeclBits.IsCopyDeductionCandidate; +return getDeductionCandidateKind() == DeductionCandidateKind::Copy; + } + + bool

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. Sorry for not getting back to you earlier. It mostly looks reasonable to me. what do you think @erichkeane ? Comment at: clang/lib/Sema/SemaInit.cpp:10338 +Context.getRValueReferenceType(ElementTypes[i]); + else if

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 483735. ychen added a comment. - update cxx_status.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files: clang/include/clang/AST/DeclBase.h

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen added a comment. 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

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 482331. ychen marked 2 inline comments as done. ychen added a comment. - Address Corentin's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files:

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

cor3ntin added a comment. 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 Comment at: clang/include/clang/AST/DeclCXX.h:1911

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen updated this revision to Diff 482088. ychen added a comment. format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139837/new/ https://reviews.llvm.org/D139837 Files: clang/include/clang/AST/DeclBase.h clang/include/clang/AST/DeclCXX.h

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

ychen created this revision. ychen added reviewers: aaron.ballman, erichkeane, rsmith. Herald added a subscriber: martong. Herald added a reviewer: shafik. Herald added a project: All. ychen requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits.