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

Allow thread_local!() to take attributes #30759

Merged
merged 2 commits into from
Feb 4, 2016
Merged

Conversation

Manishearth
Copy link
Member

fixes #30756

r? @gankro

@Gankra
Copy link
Contributor

Gankra commented Jan 7, 2016

r? @brson

(I have no background on TLS/macros/attributes)

@rust-highfive rust-highfive assigned brson and unassigned Gankra Jan 7, 2016
@alexcrichton
Copy link
Member

As mentioned in #30756 my preferred "fix" for now would be to not change the public interface and instead add some #[allow] attributes for the lints that may get triggered. I'd want to think a little more before changing the public interface here personally

@Manishearth
Copy link
Member Author

Updated so that it allows unsafe/dead code

@Manishearth
Copy link
Member Author

er, git push stopped working, updated now

@alexcrichton
Copy link
Member

Can you add a test for this as well?

@Manishearth
Copy link
Member Author

Done

thread_local!(static FOO: u8 = 1);

fn main() {
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing newline.

@eddyb
Copy link
Member

eddyb commented Jan 11, 2016

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jan 11, 2016

📌 Commit 9827dce has been approved by alexcrichton

@brson
Copy link
Contributor

brson commented Jan 12, 2016

Doesn't putting allow attributes in this macro make it unusable for anybody applying forbid(unused_code) or forbid(dead_code)?

@brson
Copy link
Contributor

brson commented Jan 12, 2016

@bors r-

Per my previous comment, putting allow attributes in this macro makes it unusable with forbid. This undermines the usefulness of forbid. Are there other places where we are embedding lint directives in macros?

@Manishearth
Copy link
Member Author

Yeah, it does. I'd fixed it in a more flexible way in d02f969 where attributes can be passed in, if that is more amenable.

@alexcrichton
Copy link
Member

Hm I thought that we had other examples of macros that did things like this, but we apparently don't. In retrospect I believe allow(dead_code) should be removed because all names are prefixed with underscores, which should prevent that lint from triggering anyway.

@Manishearth
Copy link
Member Author

A bunch of macros like bitflags do the thing where they can accept attributes. I don't see any which contain allows.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 27, 2016
@alexcrichton
Copy link
Member

The libs team discussed this during triage today and the conclusion was that the easiest fix here was just removing the unsafe function entirely. @Manishearth would you be ok updating the patch to just changing it to a safe function? The unsafety here is all synthetic as it's all an implementation detail anyway.

@alexcrichton alexcrichton removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 4, 2016
@Manishearth Manishearth force-pushed the attr-tls branch 2 times, most recently from 4623aff to 1f18441 Compare February 4, 2016 15:16
@Manishearth
Copy link
Member Author

Done. I'm not sure where the unsafe boundary should eventually end up, so you may want to have a close look.

@alexcrichton
Copy link
Member

Ah well technically this was the boundary (a function returning a 'static reference which wasn't actually 'static), but oh well.

Looks like the travis failure is legit (needs an unsafe block around a call to an intrinsic), but otherwise lgtm

@Manishearth
Copy link
Member Author

Yeah, I fixed the travis failure locally, but tests haven't run yet.

@Manishearth
Copy link
Member Author

Tests pass. r?

@alexcrichton
Copy link
Member

@bors: r+ 4b68c29

bors added a commit that referenced this pull request Feb 4, 2016
@bors
Copy link
Contributor

bors commented Feb 4, 2016

⌛ Testing commit 4b68c29 with merge c007e4a...

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.

thread_local!() can't take attributes
6 participants