- Notifications
You must be signed in to change notification settings - Fork 15.2k
[Clang] [C++26] Implement P1306R5 Expansion Statements #165195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 31 commits
f8bd65d b864b4c cb66211 34960d6 dbf5fec a9e1d55 86cab85 1deea2b a365f3e 355b761 3603a54 316f81a 9e43b8c 66ca06c 278977a 7ba15b0 fb7a000 9264ff0 b0468c7 a2b72d1 a217f90 a9f4244 bfdb5ed 6739929 ea2a347 d10d82a 1ae64d3 9b09c05 8e6a4af 84a3d71 ddede95 b188e7c 5a0c30f 61d7899 17d05ae 4965e7d 79af6ec 3642092 2ff4de2 4c2b4aa 3de01e9 8437f22 7915983 File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -171,6 +171,8 @@ C++2c Feature Support | |
| At this timem, references to constexpr and decomposition of *tuple-like* types are not supported | ||
| (only arrays and aggregates are). | ||
| | ||
| - Implemented `P1306R5 <https://wg21.link/P1306R5>`_ Expansion Statements. | ||
| Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're not changing the Feature Test Macro here, I'd suggest we elaborate here that this is a partial implementation, perhaps mentioning our concerns. | ||
| | ||
| C++23 Feature Support | ||
| ^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
| | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -3343,6 +3343,55 @@ class TemplateParamObjectDecl : public ValueDecl, | |
| static bool classofKind(Kind K) { return K == TemplateParamObject; } | ||
| }; | ||
| | ||
| /// Represents a C++26 expansion statement declaration. | ||
| /// | ||
| /// This is a bit of a hack, since expansion statements shouldn't really be | ||
| /// "declarations" per se (they don't declare anything). Nevertheless, we *do* | ||
| /// need them to be declaration *contexts*, because the DeclContext is used to | ||
| /// compute the "template depth" of entities enclosed therein. In particular, | ||
| /// the "template depth" is used to find instantiations of parameter variables, | ||
| /// and a lambda enclosed within an expansion statement cannot compute its | ||
| /// template depth without a pointer to the enclosing expansion statement. | ||
| class ExpansionStmtDecl : public Decl, public DeclContext { | ||
| CXXExpansionStmt *Expansion = nullptr; | ||
| TemplateParameterList *TParams; | ||
| CXXExpansionInstantiationStmt *Instantiations = nullptr; | ||
| Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a little odd? Or am I missing something? Why would a decl need its list of instantiations? Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the way I’ve set this up is, if the expansion statement has been expanded, we never instantiate it again and instead only instantiate the expansions (because we’d just be spending time instantiating something that will never be used for anything). We could probably also most this into Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... I see, so this is actually just a SINGLE instantiation when done in a non-dependent context? The name threw me a bit. We wouldn't expect the 'dependent' components to this to actually end up anywhere else/be changeable, right? So the process is effectively to finish this context, then 'step back' into it and do a tree-transform/instantiation? Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I’m not sure I explained that too well earlier, and the terminology here is a bit confusing (at least I keep getting confused by it when trying to explain it, because ‘instantiating’ means 2 different thigns here). To rephrase this a bit, let’s distinguish between ‘expanding’ an expansion statement (i.e. instantiating its body N times) from ‘instantiating’ the entire thing (because it just happens to be in a template, and we instantiate the containing template); what I meant was, after it’s been ‘expanded’ (which happens as soon as we parse it if the expansion size In the AST, the
The latter is only present after expansion (and it is what’s used for constant evaluation and codegen); additionally, once we’ve performed expansion, we never touch the For example, if the user writes template <typename T> void f() { template for (auto x : {T(1), T(2)}) { T a = x; } }the expansion statement is ‘expanded’ immediately (because even though template <typename T> void f() { { // Note: This outer ‘compound statement’ is actually an ExpansionStmtDecl whose // 'ExpansionInstantiationStmt' contains the following 2 compound statements: { T a = T(1); } { T a = T(2); } } }(of course, we still preserve e.g. the void f<int>() { { // Note: This outer ‘compound statement’ is the instantiated ExpansionStmtDecl. { int a = int(1); } { int a = int(2); } } }Notably, the original expansion-initializer, i.e. Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooof, all of that is pretty subtle, and I fear/suspect I'm going to spend a lot of time re-reading the above as I try to review anything derived from this. I wonder if we could start on JUST the I might also suggest trying to un-overload the 'instantiation' phrase here, nad see if we could call it 'expanded' or something like that. Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
patches that are MOSTLY mechanical for stuff like that is way easier to review.
yeah, that is unfortunate but understandable based on their concept. Perhaps leave it
Yeah, that is unfortunate. Its too bad we couldn't have talked about this before it got standardized and suggested nixing those two:) Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I should probably add (an abridged version of) my big comment above as a comment
I mean, of all the things that are complicated about this, I feel like Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I’ve added some more documentation and examples to the various AST nodes; let me know if this makes the purpose of individual nodes clearer. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you research the possibility of making non-decl decl contexts? CXXExpansionStmt : public DeclContext, public Stmt (alternatively, decoupling template depth tracking from decl context, this is very similar to our lambdas issues @mizvekov ) FYI, I fully realize I'm asking for a pony here, I just want to know if any exploration was done. In the meantime, maybe we could find a better naming scheme Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I briefly looked into that, but there are enough places where we do
I’m candidly not familiar enough with our template engine to know what exactly that would entail.
I’m pretty bad at naming so that’s definitely better than what I came up with. I’ll change it to that instead. | ||
| | ||
| ExpansionStmtDecl(DeclContext *DC, SourceLocation Loc, | ||
| TemplateParameterList *TParams); | ||
| | ||
| public: | ||
| friend class ASTDeclReader; | ||
| | ||
| static ExpansionStmtDecl *Create(ASTContext &C, DeclContext *DC, | ||
| SourceLocation Loc, | ||
| TemplateParameterList *TParams); | ||
| static ExpansionStmtDecl *CreateDeserialized(ASTContext &C, GlobalDeclID ID); | ||
| | ||
| CXXExpansionStmt *getExpansionPattern() { return Expansion; } | ||
| const CXXExpansionStmt *getExpansionPattern() const { return Expansion; } | ||
| void setExpansionPattern(CXXExpansionStmt *S) { Expansion = S; } | ||
| | ||
| CXXExpansionInstantiationStmt *getInstantiations() { return Instantiations; } | ||
| const CXXExpansionInstantiationStmt *getInstantiations() const { | ||
| return Instantiations; | ||
| } | ||
| | ||
| void setInstantiations(CXXExpansionInstantiationStmt *S) { | ||
| Instantiations = S; | ||
| } | ||
| | ||
| NonTypeTemplateParmDecl *getIndexTemplateParm() const { | ||
| return cast<NonTypeTemplateParmDecl>(TParams->getParam(0)); | ||
| } | ||
| TemplateParameterList *getTemplateParameters() const { return TParams; } | ||
| | ||
| SourceRange getSourceRange() const override LLVM_READONLY; | ||
| | ||
| static bool classof(const Decl *D) { return classofKind(D->getKind()); } | ||
| static bool classofKind(Kind K) { return K == ExpansionStmt; } | ||
| }; | ||
| | ||
| inline NamedDecl *getAsNamedDecl(TemplateParameter P) { | ||
| if (auto *PD = P.dyn_cast<TemplateTypeParmDecl *>()) | ||
| return PD; | ||
| | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we wait before backporting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; I can make it C++26 only for now