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

Attributes on generic formals #34764

Merged
merged 3 commits into from
Oct 1, 2016
Merged

Conversation

pnkfelix
Copy link
Member

First step for #34761

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb
Copy link
Member

eddyb commented Jul 11, 2016

r? @eddyb

@bors r+

@bors
Copy link
Contributor

bors commented Jul 11, 2016

📌 Commit 337073b has been approved by eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned sfackler Jul 11, 2016
@durka
Copy link
Contributor

durka commented Jul 11, 2016

plugin-breaking, no?

@eddyb
Copy link
Member

eddyb commented Jul 11, 2016

@bors r-

Ah, yes, of course. Thanks @durka. @Manishearth r=me for the next breaking batch or whatnot.

// When encounter attributes in generics list, do not yet
// know if it is attached to lifetime or to type param.
//
// So we eagerly parsing attributes in tandem with
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/parsing/parse

@bors
Copy link
Contributor

bors commented Jul 20, 2016

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

@@ -198,6 +198,7 @@ pub fn walk_lifetime<V: Visitor>(visitor: &mut V, lifetime: &Lifetime) {
pub fn walk_lifetime_def<V: Visitor>(visitor: &mut V, lifetime_def: &LifetimeDef) {
visitor.visit_lifetime(&lifetime_def.lifetime);
walk_list!(visitor, visit_lifetime, &lifetime_def.bounds);
walk_list!(visitor, visit_attribute, &*lifetime_def.attrs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why deref here, but not with the bounds?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its necessary (here and below) because &bounds is a &Vec while &attrs is a &ThinVec.

Right now, you cannot iterate over a &ThinVec directly; you need to deref the ThinVec to turn it into a [T] slice that you can then iterate over.

@dns2utf8
Copy link
Contributor

I wonder why you used deref with the walk_list! macros, but the old code does not.

@eddyb
Copy link
Member

eddyb commented Aug 1, 2016

ping @Manishearth

@Manishearth
Copy link
Member

Oh, you need this merged? Cool, I'll start up a batch.

(In the future, for high-pri or generally blocking/bitrotty things, please let me know that that's the case and I'll immediately initiate a breaking batch)

@bors
Copy link
Contributor

bors commented Aug 12, 2016

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

@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 9, 2016

ping @Manishearth ; did you end up not starting up that batch?

@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 9, 2016

or maybe I should resolve the merge conflicts. I'll do that now. :)

@Manishearth
Copy link
Member

The batch happened, but this PR wasn't in it, not sure why :/

@bors
Copy link
Contributor

bors commented Sep 10, 2016

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

I am using `ThinAttributes` rather than a vector for attributes
attached to generics, since I expect almost all lifetime and types
parameters to not carry any attributes.
@pnkfelix
Copy link
Member Author

ping @Manishearth

@Manishearth
Copy link
Member

It's in the list. Traveling right now, can't shepherd the rollup

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.

None yet

9 participants