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 old FOLLOW checking (aka check_matcher_old). #33982

Merged
merged 1 commit into from
Jun 8, 2016

Conversation

LeoTestard
Copy link
Contributor

It was supposed to be removed at the next release cycle but is still in the tree since like 6 months.
Potential breaking change, since some cases (such as #25658) will change from a warning to an error. But the warning stating that it will be a hard error in the next release has been there for 6 months now.
I think it's safe to break this code. ^_^

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@pnkfelix pnkfelix added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label May 31, 2016
@pnkfelix
Copy link
Member

triage: nominated

(Nominating since I just want to make sure lang team is aware of the long awaited breaking change here, and also because I'm not 100% sure about the protocol here.)

@LeoTestard LeoTestard force-pushed the remove-check-matcher-old branch 2 times, most recently from b44bfa2 to 9e35b01 Compare June 1, 2016 13:26
@nikomatsakis
Copy link
Contributor

Conclusion from lang-team meeting: let's follow a foreshortened protocol here, but at least post a warning to internals.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 2, 2016

posted warning to internals: link

@pnkfelix
Copy link
Member

pnkfelix commented Jun 2, 2016

cc #30450

@durka
Copy link
Contributor

durka commented Jun 4, 2016

@LeoTestard does this build locally for you? I'm trying to hack on it and getting errors in libcore that suggests macros are silently failing to expand.

check_matcher_core(cx, &first_sets, matcher, &empty_suffix, on_fail);
let err = cx.parse_sess.span_diagnostic.err_count();
check_matcher_core(cx, &first_sets, matcher, &empty_suffix);
err < cx.parse_sess.span_diagnostic.err_count()
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is backwards! < should be ==. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooooh right, I'm so stupid. ><

@LeoTestard
Copy link
Contributor Author

LeoTestard commented Jun 4, 2016

@durka I must admit that I haven't tested it and I was waiting for Travis to do it. The error you pointed out is probably the cause. The fact that it fails silently is legit because the errors are supposed to be reported directly by check_matcher_core, when present. But the return value of false will just cause the macro to be compiled in a dummy macro that expands to nothing to avoid duplicate errors. I will fix that soon. :)

@durka
Copy link
Contributor

durka commented Jun 4, 2016

You need to also replace all instances of WARN with ERROR in src/test/compile-fail/macro-follow.rs. (edit: ...and also in issue-30715.rs)

@durka
Copy link
Contributor

durka commented Jun 4, 2016

Actually the macro in issue-30715.rs is no longer legal with these changes. Maybe we should just delete that test?

@LeoTestard
Copy link
Contributor Author

r? @pnkfelix

@pnkfelix
Copy link
Member

pnkfelix commented Jun 7, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jun 7, 2016

📌 Commit 4dab8ae has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Jun 8, 2016

⌛ Testing commit 4dab8ae with merge 371bf0e...

bors added a commit that referenced this pull request Jun 8, 2016
Remove the old FOLLOW checking (aka `check_matcher_old`).

It was supposed to be removed at the next release cycle but is still in the tree since like 6 months.
Potential breaking change, since some cases (such as #25658) will change from a warning to an error. But the warning stating that it will be a hard error in the next release has been there for 6 months now.
I think it's safe to break this code. ^_^
@bors bors merged commit 4dab8ae into rust-lang:master Jun 8, 2016
@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 23, 2016
@brson
Copy link
Contributor

brson commented Jun 23, 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-lang Relevant to the language 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