Skip to content
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

Merged
merged 26 commits into from
Jan 29, 2025
Merged
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3c81c5b
[Clang][P1061] stuctured binding packs
ricejasonf Jul 22, 2021
116aff1
[Clang][P1061] Improve visiting HoldingVars; Improve parsing diagnost…
ricejasonf Jan 2, 2025
66ff7c6
[Clang][P1061] Remove unrelated code changes; Address style issues
ricejasonf Jan 2, 2025
5720bd3
[Clang][P1061] Remove vestigial changes
ricejasonf Jan 3, 2025
ffefb67
[Clang][P1061] Simplify unwrapping ResolvedPacks; other style fixes
ricejasonf Jan 3, 2025
685f947
[Clang][P1061] Add ASTReader implementation
ricejasonf Jan 4, 2025
d957783
[Clang][P1061] Improve AST dump testing
ricejasonf Jan 4, 2025
91ca5b4
[Clang][P1061] Add flat_binding_iterator
ricejasonf Jan 10, 2025
abab5cf
[Clang][P1061] Remove Decomp Visit functions
ricejasonf Jan 10, 2025
80acb71
[Clang][P1061] Use range concat for flat_bindings
ricejasonf Jan 15, 2025
360708d
[Clang][P1061] Unremove llvm_unreachable in EmitDecl
ricejasonf Jan 15, 2025
ea249ac
[Clang][P1061] Move rebuild of resolved packs into TreeTransform
ricejasonf Jan 15, 2025
dda9d89
[Clang][P1061] Fix crash
ricejasonf Jan 16, 2025
0b83b99
[Clang][P1061] Fix crash (part 2)
ricejasonf Jan 16, 2025
d496c54
[Clang][P1061] Add bit-field and pack indexing tests
ricejasonf Jan 16, 2025
d064095
[Clang][P1061] Make BindingDecl have NULL TYPE by default
ricejasonf Jan 16, 2025
97257d7
[Clang][P1061] Revert change to typo correction test
ricejasonf Jan 16, 2025
39ab76e
[Clang][P1061] Add codegen test
ricejasonf Jan 17, 2025
765db8a
[Clang][P1061] Clean up stuff
ricejasonf Jan 17, 2025
8cf8205
[Clang][P1061] Use cast_if_present
ricejasonf Jan 17, 2025
6d49a3d
[Clang][P1061] Improve error message wording
ricejasonf Jan 18, 2025
5c351d7
[Clang][P1061] Fix error messages in tests
ricejasonf Jan 18, 2025
5302830
[Clang][P1061] Remove BindingInitWalker
ricejasonf Jan 21, 2025
e497a0c
[Clang][P1061] Handle explicitly capturing binding packs
zwuis Jan 22, 2025
5751d2f
[Clang][P1016] Address nits; Remove unneeded code
ricejasonf Jan 28, 2025
36f8029
[Clang][P1061] Add proper C++ compatability warnings
ricejasonf Jan 29, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions clang/include/clang/AST/Decl.h
Original file line number Diff line number Diff line change
@@ -698,6 +698,10 @@ class ValueDecl : public NamedDecl {
return const_cast<ValueDecl *>(this)->getPotentiallyDecomposedVarDecl();
}

/// Determine whether this value is actually a function parameter pack,
/// init-capture pack, or structured binding pack
bool isParameterPack() const;

// Implement isa/cast/dyncast/etc.
static bool classof(const Decl *D) { return classofKind(D->getKind()); }
static bool classofKind(Kind K) { return K >= firstValue && K <= lastValue; }
@@ -1527,10 +1531,6 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
NonParmVarDeclBits.IsInitCapture = IC;
}

/// Determine whether this variable is actually a function parameter pack or
/// init-capture pack.
bool isParameterPack() const;

/// Whether this local extern variable declaration's previous declaration
/// was declared in the same block scope. Only correct in C++.
bool isPreviousDeclInSameBlockScope() const {
60 changes: 50 additions & 10 deletions clang/include/clang/AST/DeclCXX.h
Original file line number Diff line number Diff line change
@@ -4131,31 +4131,32 @@ class BindingDecl : public ValueDecl {
/// binding).
Expr *Binding = nullptr;

BindingDecl(DeclContext *DC, SourceLocation IdLoc, IdentifierInfo *Id)
: ValueDecl(Decl::Binding, DC, IdLoc, Id, QualType()) {}
BindingDecl(DeclContext *DC, SourceLocation IdLoc, IdentifierInfo *Id,
QualType T)
: ValueDecl(Decl::Binding, DC, IdLoc, Id, T) {}

void anchor() override;

public:
friend class ASTDeclReader;

static BindingDecl *Create(ASTContext &C, DeclContext *DC,
SourceLocation IdLoc, IdentifierInfo *Id);
SourceLocation IdLoc, IdentifierInfo *Id,
QualType T);
static BindingDecl *CreateDeserialized(ASTContext &C, GlobalDeclID ID);

/// Get the expression to which this declaration is bound. This may be null
/// in two different cases: while parsing the initializer for the
/// decomposition declaration, and when the initializer is type-dependent.
Expr *getBinding() const { return Binding; }

// Get the array of Exprs when the binding represents a pack.
llvm::ArrayRef<Expr *> getBindingPackExprs() const;

/// Get the decomposition declaration that this binding represents a
/// decomposition of.
ValueDecl *getDecomposedDecl() const { return Decomp; }

/// Get the variable (if any) that holds the value of evaluating the binding.
/// Only present for user-defined bindings for tuple-like types.
VarDecl *getHoldingVar() const;

/// Set the binding for this BindingDecl, along with its declared type (which
/// should be a possibly-cv-qualified form of the type of the binding, or a
/// reference to such a type).
@@ -4167,6 +4168,10 @@ class BindingDecl : public ValueDecl {
/// Set the decomposed variable for this BindingDecl.
void setDecomposedDecl(ValueDecl *Decomposed) { Decomp = Decomposed; }

/// Get the variable (if any) that holds the value of evaluating the binding.
/// Only present for user-defined bindings for tuple-like types.
VarDecl *getHoldingVar() const;
Copy link
Collaborator

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?


static bool classof(const Decl *D) { return classofKind(D->getKind()); }
static bool classofKind(Kind K) { return K == Decl::Binding; }
};
@@ -4194,8 +4199,16 @@ class DecompositionDecl final
NumBindings(Bindings.size()) {
std::uninitialized_copy(Bindings.begin(), Bindings.end(),
getTrailingObjects<BindingDecl *>());
for (auto *B : Bindings)
for (auto *B : Bindings) {
B->setDecomposedDecl(this);
if (B->isParameterPack()) {
for (Expr *E : B->getBindingPackExprs()) {
auto *DRE = cast<DeclRefExpr>(E);
auto *NestedB = cast<BindingDecl>(DRE->getDecl());
NestedB->setDecomposedDecl(this);
}
}
}
}

void anchor() override;
@@ -4213,8 +4226,35 @@ class DecompositionDecl final
static DecompositionDecl *CreateDeserialized(ASTContext &C, GlobalDeclID ID,
unsigned NumBindings);

ArrayRef<BindingDecl *> bindings() const {
return llvm::ArrayRef(getTrailingObjects<BindingDecl *>(), NumBindings);
// Provide the range of bindings which may have a nested pack.
llvm::ArrayRef<BindingDecl *> bindings() const {
return {getTrailingObjects<BindingDecl *>(), NumBindings};
}

// Provide a flattened range to visit each binding.
auto flat_bindings() const {
llvm::ArrayRef<BindingDecl *> Bindings = bindings();
llvm::ArrayRef<Expr *> PackExprs;

// Split the bindings into subranges split by the pack.
auto S1 = Bindings.take_until(
[](BindingDecl *BD) { return BD->isParameterPack(); });

Bindings = Bindings.drop_front(S1.size());
if (!Bindings.empty()) {
PackExprs = Bindings.front()->getBindingPackExprs();
Bindings = Bindings.drop_front();
}

auto S2 = llvm::map_range(PackExprs, [](Expr *E) {
auto *DRE = cast<DeclRefExpr>(E);
return cast<BindingDecl>(DRE->getDecl());
});

// llvm::concat must take temporaries or it will capture
// references.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this comment a bit confusing, because move produces a reference rather than a temporary. How about something like "Pass xvalues to llvm::concat to request that it makes a copy."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I just removed the comment.

return llvm::concat<BindingDecl *>(std::move(S1), std::move(S2),
std::move(Bindings));
}

void printName(raw_ostream &OS, const PrintingPolicy &Policy) const override;
53 changes: 53 additions & 0 deletions clang/include/clang/AST/ExprCXX.h
Original file line number Diff line number Diff line change
@@ -5321,6 +5321,59 @@ 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
Comment on lines +5324 to +5326
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be doing the same job as SubstNonTypeTemplateParmPackExpr and FunctionParmPackExpr. Do we need all three? If so, mentioning the relationship between the three in this comment might be nice -- but I do wonder if FunctionParmPackExpr could be generalized to cover the case of a binding declaration too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ResolvedUnexpandedPackExpr. This is originally what I had for parmexprs (P1221).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we try to address that?

: public Expr,
private llvm::TrailingObjects<ResolvedUnexpandedPackExpr, Expr *> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we store BindingDecl * in trailing objects like FunctionParmPackExpr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. That would simplify the DeclRefExpr stuff and a few other things. BindingPackExpr?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... BindingPackExpr?

This is what I think: using BindingPackExpr and modifying it when implementing other features (member packs etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I am content to leave it as is, but storing Decls instead of Exprs does have the benefit of delaying building the DeclRefExprs until expansion which creates less garbage and would simplify flat_bindings. If anything, I would leave the name ResolvedUnexpandedPackExpr and make the trailing objects ValueDecl to support future consolidation. That is premised on never wanting to create a pack of expressions which was my original intent as this was made for P1221 (which I have abandoned). I will defer to @cor3ntin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is premised on never wanting to create a pack of expressions

A use case would be unpacking a tuple for example (or data member pack but in that case maybe we want to store decl for that too)

Either way, I don't think we should predicament the design on future evolution. If we need to change it later, we can

friend class ASTStmtReader;
friend class ASTStmtWriter;
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; }

llvm::MutableArrayRef<Expr *> getExprs() {
return {getTrailingObjects<Expr *>(), NumExprs};
}

llvm::ArrayRef<Expr *> getExprs() const {
return {getTrailingObjects<Expr *>(), NumExprs};
}

Expr *getExpansion(unsigned Idx) { return getExprs()[Idx]; }
Expr *getExpansion(unsigned Idx) const { return getExprs()[Idx]; }

// Iterators
child_range children() {
return child_range((Stmt **)getTrailingObjects<Expr *>(),
(Stmt **)getTrailingObjects<Expr *>() + 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
1 change: 1 addition & 0 deletions clang/include/clang/AST/RecursiveASTVisitor.h
Original file line number Diff line number Diff line change
@@ -2936,6 +2936,7 @@ DEF_TRAVERSE_STMT(FunctionParmPackExpr, {})
DEF_TRAVERSE_STMT(CXXFoldExpr, {})
DEF_TRAVERSE_STMT(AtomicExpr, {})
DEF_TRAVERSE_STMT(CXXParenListInitExpr, {})
DEF_TRAVERSE_STMT(ResolvedUnexpandedPackExpr, {})

DEF_TRAVERSE_STMT(MaterializeTemporaryExpr, {
if (S->getLifetimeExtendedTemporaryDecl()) {
7 changes: 7 additions & 0 deletions clang/include/clang/Basic/DiagnosticParseKinds.td
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 packs in structured binding declaration">;
def note_previous_ellipsis : Note<
"previous binding pack specified here">;
def ext_cxx_binding_pack : ExtWarn<
"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<
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
@@ -5906,6 +5906,9 @@ def warn_cxx23_pack_indexing : Warning<
"pack indexing is incompatible with C++ standards before C++2c">,
DefaultIgnore, InGroup<CXXPre26Compat>;

def err_pack_outside_template : Error<
"pack declaration outside of template">;

def err_fold_expression_packs_both_sides : Error<
"binary fold expression has unexpanded parameter packs in both operands">;
def err_fold_expression_empty : Error<
1 change: 1 addition & 0 deletions clang/include/clang/Basic/StmtNodes.td
Original file line number Diff line number Diff line change
@@ -162,6 +162,7 @@ def MaterializeTemporaryExpr : StmtNode<Expr>;
def LambdaExpr : StmtNode<Expr>;
def CXXFoldExpr : StmtNode<Expr>;
def CXXParenListInitExpr: StmtNode<Expr>;
def ResolvedUnexpandedPackExpr : StmtNode<Expr>;

// C++ Coroutines expressions
def CoroutineSuspendExpr : StmtNode<Expr, 1>;
1 change: 1 addition & 0 deletions clang/include/clang/Sema/DeclSpec.h
Original file line number Diff line number Diff line change
@@ -1795,6 +1795,7 @@ class DecompositionDeclarator {
IdentifierInfo *Name;
SourceLocation NameLoc;
std::optional<ParsedAttributes> Attrs;
SourceLocation EllipsisLoc;
};

private:
4 changes: 3 additions & 1 deletion clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
@@ -230,7 +230,8 @@ void threadSafetyCleanup(BeforeSet *Cache);

// FIXME: No way to easily map from TemplateTypeParmTypes to
// TemplateTypeParmDecls, so we have this horrible PointerUnion.
typedef std::pair<llvm::PointerUnion<const TemplateTypeParmType *, NamedDecl *>,
typedef std::pair<llvm::PointerUnion<const TemplateTypeParmType *, NamedDecl *,
ResolvedUnexpandedPackExpr *>,
SourceLocation>
UnexpandedParameterPack;

@@ -6012,6 +6013,7 @@ class Sema final : public SemaBase {
RecordDecl *ClassDecl,
const IdentifierInfo *Name);

unsigned GetDecompositionElementCount(QualType DecompType);
void CheckCompleteDecompositionDeclaration(DecompositionDecl *DD);

/// Stack containing information needed when in C++2a an 'auto' is encountered
1 change: 1 addition & 0 deletions clang/include/clang/Serialization/ASTBitCodes.h
Original file line number Diff line number Diff line change
@@ -1890,6 +1890,7 @@ enum StmtCode {
EXPR_PACK_EXPANSION, // PackExpansionExpr
EXPR_PACK_INDEXING, // PackIndexingExpr
EXPR_SIZEOF_PACK, // SizeOfPackExpr
EXPR_RESOLVED_UNEXPANDED_PACK, // ResolvedUnexpandedPackExpr
EXPR_SUBST_NON_TYPE_TEMPLATE_PARM, // SubstNonTypeTemplateParmExpr
EXPR_SUBST_NON_TYPE_TEMPLATE_PARM_PACK, // SubstNonTypeTemplateParmPackExpr
EXPR_FUNCTION_PARM_PACK, // FunctionParmPackExpr
5 changes: 3 additions & 2 deletions clang/lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
@@ -12726,11 +12726,12 @@ 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 *DD = dyn_cast<DecompositionDecl>(VD)) {
for (const auto *BD : DD->flat_bindings())
if (const auto *BindingVD = BD->getHoldingVar())
if (DeclMustBeEmitted(BindingVD))
return true;
}

return false;
}
2 changes: 1 addition & 1 deletion clang/lib/AST/ASTImporter.cpp
Original file line number Diff line number Diff line change
@@ -2552,7 +2552,7 @@ ExpectedDecl ASTNodeImporter::VisitBindingDecl(BindingDecl *D) {

BindingDecl *ToD;
if (GetImportedOrCreateDecl(ToD, D, Importer.getToContext(), DC, Loc,
Name.getAsIdentifierInfo()))
Name.getAsIdentifierInfo(), D->getType()))
return ToD;

Error Err = Error::success();
11 changes: 7 additions & 4 deletions clang/lib/AST/Decl.cpp
Original file line number Diff line number Diff line change
@@ -2659,10 +2659,6 @@ bool VarDecl::checkForConstantInitialization(
return Eval->HasConstantInitialization;
}

bool VarDecl::isParameterPack() const {
return isa<PackExpansionType>(getType());
}

template<typename DeclT>
static DeclT *getDefinitionOrSelf(DeclT *D) {
assert(D);
@@ -5397,6 +5393,13 @@ bool ValueDecl::isInitCapture() const {
return false;
}

bool ValueDecl::isParameterPack() const {
if (const auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(this))
return NTTP->isParameterPack();

return isa_and_nonnull<PackExpansionType>(getType().getTypePtrOrNull());
}

void ImplicitParamDecl::anchor() {}

ImplicitParamDecl *ImplicitParamDecl::Create(ASTContext &C, DeclContext *DC,
2 changes: 1 addition & 1 deletion clang/lib/AST/DeclBase.cpp
Original file line number Diff line number Diff line change
@@ -237,7 +237,7 @@ bool Decl::isTemplateParameterPack() const {
}

bool Decl::isParameterPack() const {
if (const auto *Var = dyn_cast<VarDecl>(this))
if (const auto *Var = dyn_cast<ValueDecl>(this))
return Var->isParameterPack();

return isTemplateParameterPack();
17 changes: 13 additions & 4 deletions clang/lib/AST/DeclCXX.cpp
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,13 @@ VarDecl *BindingDecl::getHoldingVar() const {
return VD;
}

llvm::ArrayRef<Expr *> BindingDecl::getBindingPackExprs() const {
if (!Binding)
return {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at how this is used, i think we can assert here instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. The assert was being triggered in one spot that I realized was unnecessary (when I tried removing the whole block of code that was going back and adding types to the DeclRefExprs.)

auto *RP = cast<ResolvedUnexpandedPackExpr>(Binding);
return RP->getExprs();
}

void DecompositionDecl::anchor() {}

DecompositionDecl *DecompositionDecl::Create(ASTContext &C, DeclContext *DC,
1 change: 1 addition & 0 deletions clang/lib/AST/Expr.cpp
Original file line number Diff line number Diff line change
@@ -3653,6 +3653,7 @@ bool Expr::HasSideEffects(const ASTContext &Ctx,
case PackIndexingExprClass:
case HLSLOutArgExprClass:
case OpenACCAsteriskSizeExprClass:
case ResolvedUnexpandedPackExprClass:
// These never have a side-effect.
return false;

Loading