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

Move use of 'default fn' to its own module #161

Merged
merged 1 commit into from Aug 26, 2019
Merged

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Aug 20, 2019

This is the proper way to do cfg(...) gating when dealing with an unstable feature that introduces syntax.


This change is Reviewable

@emilio
Copy link
Member

emilio commented Aug 25, 2019

Can you elaborate? Why is it the right way, or better than the current code?

This moves one of the specialized functions to its own module, but not the rest, so now you need to look at two different files that interact with each other.

@Centril
Copy link
Contributor Author

Centril commented Aug 25, 2019

Can you elaborate? Why is it the right way, or better than the current code?

The way this is encoded right now assumes that rustc's parser will always be able to parse default fn even on stable. That's not an assumption you should make because it will force rustc do parse default fn forever even if we decide to remove specialization or change the syntax somehow. By moving the use of the unstable syntax into its own mod foobar; and using #[cfg(CONDITIONALLY_FALSE)] you will make libsyntax avoid parsing the contents of foobar if CONDITIONALLY_FALSE does not hold.

@emilio
Copy link
Member

emilio commented Aug 25, 2019

That seems like a pretty common footgun, isn't it? Is that something that rustc should warn about somehow?

@Centril
Copy link
Contributor Author

Centril commented Aug 25, 2019

Not that common I think but I'm working on doing more pre-expansion gating so you don't get into this situation in most cases.

Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

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

I guess I can't stamp it, though not in love with this :)

@emilio
Copy link
Member

emilio commented Aug 26, 2019

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 92122e2 has been approved by emilio

bors-servo pushed a commit that referenced this pull request Aug 26, 2019
Move use of 'default fn' to its own module

This is the proper way to do `cfg(...)` gating when dealing with an unstable feature that introduces syntax.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/161)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

⌛ Testing commit 92122e2 with merge 3ee6d1e...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: emilio
Pushing 3ee6d1e to master...

@bors-servo bors-servo merged commit 92122e2 into servo:master Aug 26, 2019
mbrubeck added a commit that referenced this pull request Oct 30, 2019
* Move code using specialization into its own module (#161)
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

3 participants