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

Check attribute usage #28650

Merged
merged 2 commits into from
Oct 2, 2015
Merged

Check attribute usage #28650

merged 2 commits into from
Oct 2, 2015

Conversation

sanxiyn
Copy link
Member

@sanxiyn sanxiyn commented Sep 25, 2015

This is technically a [breaking-change].

Fix #2809.
Fix #22746.

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@sanxiyn sanxiyn force-pushed the attr-usage branch 2 times, most recently from ab80e64 to fb22683 Compare September 25, 2015 09:50
@alexcrichton
Copy link
Member

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned arielb1 Sep 25, 2015
@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 25, 2015
@alexcrichton
Copy link
Member

Starting a crater build to evaluate the impact

@alexcrichton
Copy link
Member

Looks like there's only one regression, so should be easy to send a PR for!

@nagisa
Copy link
Member

nagisa commented Sep 25, 2015

I’d replace error message wording from “should” to “may only” or “may”.

Otherwise always 👍 for such correctness PRs.

if words.is_none() {
return;
}
let words = words.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

nit:

let words = match attr.meta_item_list() {
    Some(words) => words,
    None => {
        return;
    }
};

@nrc
Copy link
Member

nrc commented Sep 27, 2015

r=me with the style nit fixed

@sanxiyn sanxiyn force-pushed the attr-usage branch 2 times, most recently from c04e6d9 to c5a7d17 Compare September 28, 2015 04:54
@sanxiyn
Copy link
Member Author

sanxiyn commented Sep 28, 2015

Done.

@nrc
Copy link
Member

nrc commented Sep 28, 2015

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 28, 2015

📌 Commit c5a7d17 has been approved by nrc

@bors
Copy link
Contributor

bors commented Sep 28, 2015

⌛ Testing commit c5a7d17 with merge 186df85...

@bors
Copy link
Contributor

bors commented Sep 28, 2015

💔 Test failed - auto-mac-32-opt

@sanxiyn
Copy link
Member Author

sanxiyn commented Oct 2, 2015

@bors r=nrc

@bors
Copy link
Contributor

bors commented Oct 2, 2015

📌 Commit 61f5b2b has been approved by nrc

bors added a commit that referenced this pull request Oct 2, 2015
This is technically a [breaking-change].

Fix #2809.
Fix #22746.
@bors
Copy link
Contributor

bors commented Oct 2, 2015

⌛ Testing commit 61f5b2b with merge ef07d7d...

@bors bors merged commit 61f5b2b into rust-lang:master Oct 2, 2015
@sanxiyn sanxiyn deleted the attr-usage branch October 5, 2015 04:19
@Manishearth
Copy link
Member

IMO we should have made this a warning for one release cycle, but I guess this is fine too.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants