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

Add notes on migrating crates to work with both editions #117

Merged
merged 8 commits into from
Nov 10, 2018

Conversation

richard-uk1
Copy link
Contributor

Some notes on supporting both editions while using implementation macros.

I haven't checked that the code compiles, so I would be grateful for a thorough review!

Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

This looks good to me, though I haven't compiled it either. @dtolnay if you have some time, can you check this out?


#[doc(hidden)]
#[macro_export]
macro_rules! log {
Copy link
Member

Choose a reason for hiding this comment

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

I would rename this macro to private_log! or log_impl! or __log! to make it clear that importing it is not a reasonable fix (because the real log crate does export a public log! macro).

```rust,ignore
#[doc(hidden)]
#[macro_export]
macro_rules! my_special_stringify {
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to see my_special_... in an actual crate. Could you come up with a naming convention that we can recommend to all uses of this pattern?

#[macro_export]
macro_rules! my_special_stringify {
($($inner:tt)*) => {
stringify!($($inner)*)
Copy link
Member

Choose a reason for hiding this comment

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

Since the approach described here is intended to be picked up by all sorts of macros, I would write this call with curly braces.

    stringify! { $($inner)* }

Some macros cannot be forwarded with parentheses: playground. All macros can be forwarded with curly braces.

#[macro_export]
macro_rules! warn {
($msg:expr) => {
log!(stringify!($crate::LogLevel::Warn), $msg)
Copy link
Member

Choose a reason for hiding this comment

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

Please pick a different standard library macro to build this demo code around. You would never see stringify! called like this on a crate local path. format_args! could work better.

@richard-uk1
Copy link
Contributor Author

I've made these changes, and turned on CI for the last example, since it should compile in all situations :)

@richard-uk1
Copy link
Contributor Author

This is ready for review again.


So the code knows to look for any macros used locally. But wait - this won't compile, because we
use the `format_args!` macro that isn't in our local crate (hence the convoluted example). The
solution is to add a level of indirection: we crate a macro that wraps `format_args`, but is local

Choose a reason for hiding this comment

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

typo here we crate => we create

@@ -78,3 +80,192 @@ struct Bar;

This only works for macros defined in external crates.
For macros defined locally, `#[macro_use] mod foo;` is still required, as it was in Rust 2015.

### Local helper macros

Choose a reason for hiding this comment

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

It may be useful for authors of crates using foreign helper macros to also have some examples. Those examples may almost look exactly the local examples below so it may be even better to rename this sections helper macros with examples using a mix of both foreign-crate:: and $crate:: alike.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be possible to merge this PR first, and then address this in a new PR?

Choose a reason for hiding this comment

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

Most definitely. I'm just a passer by that was pointed at this PR for documentation on the new feature. No blockers here

crate -> create
@richard-uk1
Copy link
Contributor Author

Ping - I think this will be useful to rust users so we should either merge or tell me what I need to change.

@Michael-F-Bryan
Copy link

I just ran into this problem when using the new some_crate::some_macro!() syntax and this PR would have saved me a fair amount of googling. @derekdreery is there anything left to do on this?

@petrochenkov
Copy link
Contributor

I'd merge this long time ago, but I don't have rights in this repo unfortunately.

ping @steveklabnik

@aturon aturon merged commit afef680 into rust-lang:master Nov 10, 2018
@richard-uk1
Copy link
Contributor Author

woot

@richard-uk1 richard-uk1 deleted the macro_migration branch November 10, 2018 16:58
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.

8 participants