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

Implement .. in tuple (struct) patterns (RFC 1492) #33639

Merged
merged 4 commits into from
May 27, 2016

Conversation

petrochenkov
Copy link
Contributor

cc #33627
r? @nikomatsakis

plugin-[breaking-change] cc #31645 @Manishearth

@Manishearth
Copy link
Member

Manishearth commented May 15, 2016

Since this implements an RfC, it will need to land soon, right? In that case, we probably should make a batch soon after this gets approved -- so if you have other PRs you want to make, now is the time!

@bors
Copy link
Contributor

bors commented May 18, 2016

☔ The latest upstream changes (presumably #33654) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
Copy link
Contributor Author

Rebased.

/// "None" means a `Variant(..)` pattern where we don't bind the fields to names.
TupleStruct(Path, Option<HirVec<P<Pat>>>),
/// A tuple struct/variant pattern `Variant(x, y, .., z)`.
/// If the `..` pattern fragment presents, then `Option<usize>` denotes its position.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/presents/is present/

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that there is anything wrong with it, but I am surprised by this choice; I would have expected Vec<P<Pat>> and Option<Vec<P<Pat>>>, where the second set of patterns come after the ... Did you consider this and find that the current optional usize works out better (or just that you prefer it)? Anyway, it seems ok as is, just wanted to toss an alternative out there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One allocation, the variant itself is more compact (6 ptrs vs 4 ptrs), simpler traversal (no vec1.chain(vec2)).

@nikomatsakis
Copy link
Contributor

Looks good. r=me with the add'l tests listed and nits addressed, though we should decide whether S(foo, ..,) is legal syntax.

@petrochenkov
Copy link
Contributor Author

Updated.
(pat, ..,) is still illegal, but that can be relaxed at any moment.

@aturon
Copy link
Member

aturon commented May 23, 2016

My vote: let's keep it illegal and thereby start conservatively.

@pnkfelix
Copy link
Member

I concur with @aturon; plus both syntaxes are feature gated so even if change wasn't a conservative extension we would still be within bounds to change it later

@nikomatsakis
Copy link
Contributor

OK. Seems weird to me to permit trailing commas but only sometimes, but I bow to the will of the masses!

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 23, 2016

📌 Commit 4983025 has been approved by nikomatsakis

@Manishearth
Copy link
Member

@bors r-

hold on, plugin breaking. I'll batch these up tomorrow morning

Only #33351 needs to be included for now if someone else wants to do it

@nikomatsakis
Copy link
Contributor

D'oh, sorry @Manishearth
On May 23, 2016 2:32 PM, "Manish Goregaokar" notifications@github.com
wrote:

@bors https://github.com/bors r-

hold on, plugin breaking. I'll batch these up tomorrow morning

Only #33351 #33351 needs to be
included for now if someone else wants to do it


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#33639 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants