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

Remove the plugin? #124

Closed
SimonSapin opened this issue Nov 19, 2015 · 6 comments
Closed

Remove the plugin? #124

SimonSapin opened this issue Nov 19, 2015 · 6 comments

Comments

@SimonSapin
Copy link
Member

The string_cache_plugin crate provides atom! and ns! syntax extension that turns static atoms into their interned representation at compile time. In #95 I’ve introduced an alternative: the string_cache crate has a build script that generates a giant (couple thousand lines) macro_rules! macro:

https://gist.github.com/anonymous/1f4638851d6e3c8f1975#file-ns_atom_macros_without_plugin-rs-L326

It’s ugly, but it runs on stable Rust while the plugin breaks and needs to be updated when compile internals change.

I’m tempted to remove the plugin and change every downstream user to use the macro. Or is there still a reason to prefer the plugin? CC @Manishearth @pcwalton @frewsxcv

@asajeffrey
Copy link
Member

Can we do case-insensitive atoms without expanding out the O(2^n) cases?

@frewsxcv
Copy link
Contributor

I don't feel strongly about either direction.

@SimonSapin
Copy link
Member Author

Not without a compiler plugin, I don’t think. We could also remove case-insensitivity (which is only in ns!, not atom!) and only accept all-lowercase and all-uppercase namespace prefixes.

@asajeffrey
Copy link
Member

This only impacts our own code -- what would happen if we made it a compile-time error for ns!(...) to be called with anything other than a lower-case string?

@SimonSapin
Copy link
Member Author

Or all-lowercase only! I’m ok with that. It’s a breaking change, but easy to fix.

And while we’re making breaking changes, should we remove Atom::as_slice (use Deref or AsRef impls instead) and replace Atom::from_slice with a std::convert::From impl?

@asajeffrey
Copy link
Member

These changes sound sensible, and in line with DOMString::from(string) and String::from(domstring). This is a separate issue though.

bors-servo pushed a commit that referenced this issue Nov 23, 2015
Merge into a single crate. Use macros even on unstable.

Breaking changes:

* `ns!("")` should be written `ns!()`
* Other `ns!(…)` macros should be lowercase, unquoted.

Fixes #124
Closes #123

r? @asajeffrey

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/string-cache/125)
<!-- Reviewable:end -->
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

No branches or pull requests

3 participants