-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
[Clang][P1061] Add stuctured binding packs #121417
Changes from 3 commits
3c81c5b
116aff1
66ff7c6
5720bd3
ffefb67
685f947
d957783
91ca5b4
abab5cf
80acb71
360708d
ea249ac
dda9d89
0b83b99
d496c54
d064095
97257d7
39ab76e
765db8a
8cf8205
6d49a3d
5c351d7
5302830
e497a0c
5751d2f
36f8029
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 | ||||
---|---|---|---|---|---|---|
|
@@ -5321,6 +5321,56 @@ class BuiltinBitCastExpr final | |||||
} | ||||||
}; | ||||||
|
||||||
// Represents an unexpanded pack where the list of expressions are | ||||||
// known. These are used when structured bindings introduce a pack. | ||||||
class ResolvedUnexpandedPackExpr final | ||||||
cor3ntin marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+5324
to
+5326
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 to be doing the same job as 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 definitely think they could be generalized which is what I had in mind with 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. Should we try to address that? |
||||||
: public Expr, | ||||||
private llvm::TrailingObjects<ResolvedUnexpandedPackExpr, Stmt *> { | ||||||
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.
Suggested change
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. The only place you're using this as a Stmt is for children, and the cast that way is 'safe' (and Expr isa Stmt). This change likely saves a bunch of casting elsewhere. |
||||||
friend TrailingObjects; | ||||||
|
||||||
SourceLocation BeginLoc; | ||||||
unsigned NumExprs; | ||||||
|
||||||
ResolvedUnexpandedPackExpr(SourceLocation BL, QualType QT, unsigned NumExprs); | ||||||
|
||||||
public: | ||||||
static ResolvedUnexpandedPackExpr *CreateDeserialized(ASTContext &C, | ||||||
unsigned NumExprs); | ||||||
static ResolvedUnexpandedPackExpr * | ||||||
Create(ASTContext &C, SourceLocation BeginLoc, QualType T, unsigned NumExprs); | ||||||
static ResolvedUnexpandedPackExpr *Create(ASTContext &C, | ||||||
SourceLocation BeginLoc, QualType T, | ||||||
llvm::ArrayRef<Expr *> Exprs); | ||||||
|
||||||
unsigned getNumExprs() const { return NumExprs; } | ||||||
|
||||||
Expr **getExprs() { | ||||||
return reinterpret_cast<Expr **>(getTrailingObjects<Stmt *>()); | ||||||
} | ||||||
Expr *const *getExprs() const { | ||||||
return reinterpret_cast<Expr *const *>(getTrailingObjects<Stmt *>()); | ||||||
} | ||||||
|
||||||
Expr *getExpansion(unsigned Idx) { return getExprs()[Idx]; } | ||||||
Expr *getExpansion(unsigned Idx) const { return getExprs()[Idx]; } | ||||||
|
||||||
// Iterators | ||||||
child_range children() { | ||||||
return child_range(getTrailingObjects<Stmt *>(), | ||||||
getTrailingObjects<Stmt *>() + getNumExprs()); | ||||||
} | ||||||
|
||||||
SourceLocation getBeginLoc() const LLVM_READONLY { return BeginLoc; } | ||||||
SourceLocation getEndLoc() const LLVM_READONLY { return BeginLoc; } | ||||||
|
||||||
// Returns the resolved pack of a decl or nullptr | ||||||
static ResolvedUnexpandedPackExpr *getFromDecl(Decl *); | ||||||
|
||||||
static bool classof(const Stmt *T) { | ||||||
return T->getStmtClass() == ResolvedUnexpandedPackExprClass; | ||||||
} | ||||||
}; | ||||||
|
||||||
} // namespace clang | ||||||
|
||||||
#endif // LLVM_CLANG_AST_EXPRCXX_H |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1099,6 +1099,13 @@ def err_lambda_capture_misplaced_ellipsis : Error< | |
"the name of the capture">; | ||
def err_lambda_capture_multiple_ellipses : Error< | ||
"multiple ellipses in pack capture">; | ||
def err_binding_multiple_ellipses : Error< | ||
"multiple ellipses in structured binding declaration">; | ||
def note_previous_ellipsis : Note< | ||
"previous ellipsis specified here">; | ||
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. It seems a little strange to talk about ellipses rather than packs here. Maybe "multiple binding packs in [...]" instead? |
||
def ext_cxx_binding_pack : ExtWarn< | ||
cor3ntin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"structured binding pack is incompatible with C++ standards before C++2c">, | ||
InGroup<CXX26>; | ||
def err_capture_default_first : Error< | ||
"capture default must be first">; | ||
def ext_decl_attrs_on_lambda : ExtWarn< | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12726,11 +12726,15 @@ bool ASTContext::DeclMustBeEmitted(const Decl *D) { | |
|
||
// Likewise, variables with tuple-like bindings are required if their | ||
// bindings have side-effects. | ||
if (const auto *DD = dyn_cast<DecompositionDecl>(VD)) | ||
for (const auto *BD : DD->bindings()) | ||
if (const auto *BindingVD = BD->getHoldingVar()) | ||
if (DeclMustBeEmitted(BindingVD)) | ||
return true; | ||
if (const auto *DD = dyn_cast<DecompositionDecl>(VD)) { | ||
bool BindingResult = false; | ||
DD->VisitHoldingVars([&](VarDecl *BindingVD) { | ||
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. Instead of a 'Visit' here, I wonder if there is value with some sort of 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 just added a flat_binding_iterator to replace these Visit* functions. I forgot to use it here, but if a custom iterator is what you wanted, please consider it. 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'd be a little surprised if there wasn't an existing iterator type that would work. If not though, I'd vastly prefer removing both of the 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 replaced the Visit functions with |
||
if (DeclMustBeEmitted(BindingVD)) | ||
BindingResult = true; | ||
}); | ||
if (BindingResult) | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3395,19 +3395,21 @@ VarDecl *ValueDecl::getPotentiallyDecomposedVarDecl() { | |
if (auto *Var = llvm::dyn_cast<VarDecl>(this)) | ||
return Var; | ||
if (auto *BD = llvm::dyn_cast<BindingDecl>(this)) | ||
return llvm::dyn_cast<VarDecl>(BD->getDecomposedDecl()); | ||
return llvm::dyn_cast_if_present<VarDecl>(BD->getDecomposedDecl()); | ||
return nullptr; | ||
} | ||
|
||
void BindingDecl::anchor() {} | ||
|
||
BindingDecl *BindingDecl::Create(ASTContext &C, DeclContext *DC, | ||
SourceLocation IdLoc, IdentifierInfo *Id) { | ||
return new (C, DC) BindingDecl(DC, IdLoc, Id); | ||
SourceLocation IdLoc, IdentifierInfo *Id, | ||
QualType T) { | ||
return new (C, DC) BindingDecl(DC, IdLoc, Id, T); | ||
} | ||
|
||
BindingDecl *BindingDecl::CreateDeserialized(ASTContext &C, GlobalDeclID ID) { | ||
return new (C, ID) BindingDecl(nullptr, SourceLocation(), nullptr); | ||
return new (C, ID) | ||
BindingDecl(nullptr, SourceLocation(), nullptr, QualType()); | ||
} | ||
|
||
VarDecl *BindingDecl::getHoldingVar() const { | ||
|
@@ -3423,6 +3425,30 @@ VarDecl *BindingDecl::getHoldingVar() const { | |
return VD; | ||
} | ||
|
||
void DecompositionDecl::VisitHoldingVars( | ||
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. Rather than both of these functions, I'd prefer we have some sort of iterators that allow standard iteration over these. We have 'filter' iterators already that take a lambda that we could define to allow going through these. The visitation can be nice, but it has the requirement of going through every element even when we only care about 'the first of'. |
||
llvm::function_ref<void(VarDecl *)> F) const { | ||
VisitBindings([&](BindingDecl *BD) { | ||
if (VarDecl *VD = BD->getHoldingVar()) | ||
F(VD); | ||
}); | ||
} | ||
|
||
void DecompositionDecl::VisitBindings( | ||
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 one is a bit more complicated? But I'd be surprised if there isn't an existing llvm iterator that can do this. |
||
llvm::function_ref<void(BindingDecl *)> F) const { | ||
for (BindingDecl *B : bindings()) { | ||
llvm::ArrayRef<Expr *> Exprs; | ||
if (B->isParameterPack()) { | ||
auto *RP = cast<ResolvedUnexpandedPackExpr>(B->getBinding()); | ||
Exprs = llvm::ArrayRef(RP->getExprs(), RP->getNumExprs()); | ||
for (Expr *E : Exprs) { | ||
auto *DRE = cast<DeclRefExpr>(E); | ||
F(cast<BindingDecl>(DRE->getDecl())); | ||
} | ||
} else | ||
F(B); | ||
} | ||
} | ||
|
||
void DecompositionDecl::anchor() {} | ||
|
||
DecompositionDecl *DecompositionDecl::Create(ASTContext &C, DeclContext *DC, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1965,3 +1965,52 @@ CXXFoldExpr::CXXFoldExpr(QualType T, UnresolvedLookupExpr *Callee, | |
SubExprs[SubExpr::RHS] = RHS; | ||
setDependence(computeDependence(this)); | ||
} | ||
|
||
ResolvedUnexpandedPackExpr::ResolvedUnexpandedPackExpr(SourceLocation BL, | ||
QualType QT, | ||
unsigned NumExprs) | ||
: Expr(ResolvedUnexpandedPackExprClass, QT, VK_PRValue, OK_Ordinary), | ||
BeginLoc(BL), NumExprs(NumExprs) { | ||
// C++ [temp.dep.expr]p3 | ||
// An id-expression is type-dependent if it is | ||
// - associated by name lookup with a pack | ||
setDependence(ExprDependence::TypeValueInstantiation | | ||
ExprDependence::UnexpandedPack); | ||
cor3ntin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
ResolvedUnexpandedPackExpr * | ||
ResolvedUnexpandedPackExpr::CreateDeserialized(ASTContext &Ctx, | ||
unsigned NumExprs) { | ||
void *Mem = Ctx.Allocate(totalSizeToAlloc<Stmt *>(NumExprs), | ||
alignof(ResolvedUnexpandedPackExpr)); | ||
return new (Mem) | ||
ResolvedUnexpandedPackExpr(SourceLocation(), QualType(), NumExprs); | ||
} | ||
|
||
ResolvedUnexpandedPackExpr * | ||
ResolvedUnexpandedPackExpr::Create(ASTContext &Ctx, SourceLocation BL, | ||
QualType T, unsigned NumExprs) { | ||
void *Mem = Ctx.Allocate(totalSizeToAlloc<Stmt *>(NumExprs), | ||
alignof(ResolvedUnexpandedPackExpr)); | ||
ResolvedUnexpandedPackExpr *New = | ||
new (Mem) ResolvedUnexpandedPackExpr(BL, T, NumExprs); | ||
|
||
auto Exprs = llvm::MutableArrayRef(New->getExprs(), New->getNumExprs()); | ||
std::fill(Exprs.begin(), Exprs.end(), nullptr); | ||
|
||
return New; | ||
} | ||
|
||
ResolvedUnexpandedPackExpr * | ||
ResolvedUnexpandedPackExpr::Create(ASTContext &Ctx, SourceLocation BL, | ||
QualType T, ArrayRef<Expr *> Exprs) { | ||
auto *New = Create(Ctx, BL, T, Exprs.size()); | ||
std::copy(Exprs.begin(), Exprs.end(), New->getExprs()); | ||
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. Does this need to be a 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 suppose it could. The other Create function it is calling here is using 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. IIRC I looked it up at one point and found that std::copy into uninitialized data (or perhaps std::fill as well), but I cannot for the life of me remember what it was. Perhaps it is just 'dtor' related stuff (which makes this not matter for pointers?) but the code I wrote is pointers-only too, so IDK why it became a bit of my personal cargo-cult. |
||
return New; | ||
} | ||
|
||
ResolvedUnexpandedPackExpr *ResolvedUnexpandedPackExpr::getFromDecl(Decl *D) { | ||
if (auto *BD = dyn_cast<BindingDecl>(D)) | ||
return dyn_cast_if_present<ResolvedUnexpandedPackExpr>(BD->getBinding()); | ||
return nullptr; | ||
} |
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 get the comment to move as well? Or at least an updated one?