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 #[macro_escape]. #36603

Closed
wants to merge 1 commit into from

Conversation

jseyfried
Copy link
Contributor

Remove #[macro_escape], which was deprecated at the end of 2014 (c.f 5bf385b).
#[macro_escape] is allowed with a deprecation warning on stable, so this is a [breaking-change].
r? @eddyb

@jseyfried
Copy link
Contributor Author

cc @petrochenkov

@eddyb
Copy link
Member

eddyb commented Sep 20, 2016

LGTM. @rust-lang/compiler, looks like this should've been removed before 1.0 - can we do that now?

@nrc
Copy link
Member

nrc commented Sep 20, 2016

+1

@bors
Copy link
Contributor

bors commented Sep 28, 2016

☔ The latest upstream changes (presumably #36601) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

ping r? @eddyb, perhaps this should be nominated for a compiler team discussion?

@eddyb eddyb added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 1, 2016
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 3, 2016
@nikomatsakis
Copy link
Contributor

This is really a @rust-lang/compiler team problem.

Wearing my compiler hat, I propose that we do a quick crater run to see if anyone is affected and remove it. Basically the normal bug fix protocol. At worst, we can have a warning period, but this seems like one of those cases where it's probably not needed.

@nikomatsakis
Copy link
Contributor

We can actually skip crater and just (rip)grep the sources of crates.io I guess, even easier.

@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 9, 2016
@brson
Copy link
Contributor

brson commented Nov 9, 2016

This looks like it removes all tests around #[macro_escape]. Can it instead add a test that captures what does happen when you mention macro_escape? Is it an error?

@brson
Copy link
Contributor

brson commented Nov 9, 2016

@jseyfried
Copy link
Contributor Author

jseyfried commented Nov 9, 2016

@brson Currently, #[macro_escape] would have no effect, but I think it should be a custom_attribute feature error like other unknown attributes (I'll update and add a test when we want to land).

@nikomatsakis

At worst, we can have a warning period

It has been a deprecation warning since the beginning of 2015.

At least one of the grepped crates (immutable) is a real regression (many are already broken or have a dead GH repository link). There's very little downside to keeping #[macro_escape], so I'm going to close this PR until there are no regressing crates.

@jseyfried jseyfried closed this Nov 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants