Skip to content

fix for new attributes failing issue #22964 #23056

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

Merged
merged 1 commit into from
Mar 7, 2015
Merged

fix for new attributes failing issue #22964 #23056

merged 1 commit into from
Mar 7, 2015

Conversation

awlnx
Copy link
Contributor

@awlnx awlnx commented Mar 5, 2015

No description provided.

@rust-highfive
Copy link
Contributor

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. 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 CONTRIBUTING.md for more information.

@nrc
Copy link
Member

nrc commented Mar 5, 2015

I'm not clear on why these cfgs shouldn't be removed on snapshot? Isn't bootstrapping then done? Perhaps I don't understand the motivation here.

@nrc
Copy link
Member

nrc commented Mar 5, 2015

cc @Manishearth

@awlnx
Copy link
Contributor Author

awlnx commented Mar 5, 2015

from my limited understanding during stage0 a new rust attribute isn't recognized. So we don't fail for #[foo] until stage1. bootstrap might of been the wrong word to use.e

@awlnx
Copy link
Contributor Author

awlnx commented Mar 5, 2015

cc @huonw

@nrc
Copy link
Member

nrc commented Mar 5, 2015

What is the motivation for using these blanket cfgs, rather than a cfg(not(stage0)) when you use a new attribute (like we do with everything else that is a struggle due to bootstrapping)?

@awlnx
Copy link
Contributor Author

awlnx commented Mar 5, 2015

@huonw would probably be the best person to ask about this. He pointed out over irc that @Manishearth proposed fix #[cfg(not(stage0))] would have some sort of staging issue.

@Manishearth
Copy link
Member

@nrc This is because we want people to be able to add new custom attributes easily. The current process is:

  • Add them to libsyntax/feature_gate.rs
  • Wait for a snapshot, since the wrong feature_gate.rs list is used at stage0
  • Continue

This is a permanent fix allowing them to use these features without any issues at stage0.

@nrc
Copy link
Member

nrc commented Mar 5, 2015

@Manishearth That is the same procedure for making any compiler changes, not really sure why we should give special treatment to new attributes

@Manishearth
Copy link
Member

Because custom_attribute isn't supposed to apply to the compiler anyway. It's for back compat only, so it's annoying to have this break what is otherwise a straightforward workflow.

@Manishearth
Copy link
Member

Basically, this removes an extra annoyance from bootstrapping and makes us require less snapshots.

@nrc
Copy link
Member

nrc commented Mar 5, 2015

I don't think it means fewer snapshots, it means you get to avoid faffing around with cfgs, which is still a good thing (because it is so annoying). I do worry that this is a bit of a footgun every time someone does a snapshot (it means they're less automatic to do). I also like having a uniform process rather than exceptions, even if those exceptions make things easier.

Still it is simple to rewind, and look likes a win overall, so @bors: r+ 1e68dfd

@nrc
Copy link
Member

nrc commented Mar 5, 2015

Oh, I guess squashing the commits would be better too

@nrc
Copy link
Member

nrc commented Mar 5, 2015

@bors: r-

@nrc
Copy link
Member

nrc commented Mar 5, 2015

Sorry, please get rid of the merge commit - rebase and squash the commits please, r+ with that

@awlnx
Copy link
Contributor Author

awlnx commented Mar 5, 2015

@nrc all finished up

@Manishearth
Copy link
Member

@bors: rollup r=nrc

@bors
Copy link
Collaborator

bors commented Mar 5, 2015

@bors r=nrc 951ef9d

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 6, 2015
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 6, 2015
@bors bors merged commit 951ef9d into rust-lang:master Mar 7, 2015
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.

5 participants