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

Changed macros to work on stable (with proc-macro-hack) #149

Merged
merged 1 commit into from Jul 5, 2019

Conversation

danielhenrymantilla
Copy link

proc-macro-hack-based (much simpler) reimplementation of #148, to address #106

@sfackler
Copy link
Collaborator

@dtolnay is the missing documentation warning expected?

@dtolnay
Copy link

dtolnay commented Apr 16, 2019

Yes, you have to write the documentation. ;)

/// ...
#[cfg(feature = "macros")]
#[proc_macro_hack]
pub use phf_macros::phf_map;

...

@Speedy37
Copy link

What is blocking you from merging this and stabilizing it?

Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

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

LGTM minus the version number edits.

phf/Cargo.toml Outdated Show resolved Hide resolved
@danielhenrymantilla
Copy link
Author

danielhenrymantilla commented Jul 5, 2019

Reverted the version bump, rebased on master, and added some documentation on the macros.

Also added a field in Cargo.toml to ensure that the macros documentation appears on https://docs.rs.

Remaining question: should the "macros" feature by enabled by default? I haven't done it, but except maybe for compatibility with older rustc compilers, I can't see a reason for them no to be enabled by default, given that they are stable.

@abonander
Copy link
Collaborator

We've technically bumped the minimum to 1.31 now but that's mostly to use const fn on stable with unicase. Upgrading siphasher apparently needed a new minimum version but I'm not sure what it was. So macros on by default should be fine; I'd say we could remove the feature flag entirely but if people aren't going to use it then they might as well be able to turn it off to save a little bit of compile time.

@abonander abonander merged commit ae649cd into rust-phf:master Jul 5, 2019
@abonander
Copy link
Collaborator

I can make that change myself, thanks for your contribution!

@danielhenrymantilla
Copy link
Author

I'm glad I could help. Now updating the README.md would be a good thing to do :)

@abonander
Copy link
Collaborator

Yep.

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

5 participants