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

Better attribute and metaitem encapsulation throughout the compiler #34842

Merged
merged 2 commits into from
Jul 30, 2016

Conversation

cgswords
Copy link
Contributor

This PR refactors most (hopefully all?) of the MetaItem interactions outside of libsyntax (and a few inside) to interact with MetaItems through the provided traits instead of directly creating / destruct / matching against them. This is a necessary first step to eventually converting MetaItems to internally use TokenStream representations (which will make MetaItem interactions much nicer for macro writers once the new macro system is in place).

r? @nrc

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@eddyb
Copy link
Member

eddyb commented Jul 16, 2016

cc @Manishearth

@Manishearth
Copy link
Member

Shouldn't be breaking cc @dtolnay

}
};
let metas = if let Some(metas) = meta.meta_item_list() {
metas
Copy link
Member

Choose a reason for hiding this comment

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

I think should be indented just once.

@bors
Copy link
Contributor

bors commented Jul 17, 2016

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

// ast::LitKind::FloatUnsuffixed(ref f) => f.to_string(),
// ast::LitKind::Bool(b) => b.to_string(),
// }
// }
Copy link
Member

Choose a reason for hiding this comment

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

If we don't need this code then delete it.

@cgswords cgswords force-pushed the attr_enc branch 2 times, most recently from 2bacc2e to addbc02 Compare July 18, 2016 18:56
} else {
out.push(Err(meta.span));
return out;
};
Copy link
Member

Choose a reason for hiding this comment

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

nit: 2-space indent

@nrc
Copy link
Member

nrc commented Jul 18, 2016

r = me with indenting nit and tests fixed

@nrc
Copy link
Member

nrc commented Jul 19, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit 0518021 has been approved by nrc

@bors
Copy link
Contributor

bors commented Jul 20, 2016

⌛ Testing commit 0518021 with merge 8a98905...

@bors
Copy link
Contributor

bors commented Jul 20, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Jul 19, 2016 at 9:27 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1826


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#34842 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95F6bd-PHJQi_q9vErh07R8GYgcGuks5qXaOqgaJpZM4JN1pC
.

@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.

@bors
Copy link
Contributor

bors commented Jul 21, 2016

🔒 Merge conflict

@cgswords
Copy link
Contributor Author

@alexcrichton Will you kick off bors again?

@alexcrichton
Copy link
Member

@bors: retry

@alexcrichton
Copy link
Member

er,

@bors: r=nrc 5553901

@bors
Copy link
Contributor

bors commented Jul 29, 2016

⌛ Testing commit 5553901 with merge f164cf5...

bors added a commit that referenced this pull request Jul 29, 2016
Better attribute and metaitem encapsulation throughout the compiler

This PR refactors most (hopefully all?) of the `MetaItem` interactions outside of `libsyntax` (and a few inside) to interact with MetaItems through the provided traits instead of directly creating / destruct / matching against them. This is a necessary first step to eventually converting `MetaItem`s to internally use `TokenStream` representations (which will make `MetaItem` interactions much nicer for macro writers once the new macro system is in place).

r? @nrc
@bors bors merged commit 5553901 into rust-lang:master Jul 30, 2016
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.

7 participants