upgrade thread_local! invocation syntax #34077

durka · 2016-06-04T19:20:09Z

Allows declaring multiple statics in one macro invocation, and supports attaching attributes to the generated items. In particular, #![forbid(missing_docs, unused)] is now tenable on a crate/module containing thread locals.

For an example see here. This change is fully backwards compatible as far as I can tell.

cc @frankmcsherry

rust-highfive · 2016-06-04T19:20:13Z

r? @aturon

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

durka · 2016-06-04T19:22:31Z

src/libstd/thread/local.rs

@@ -106,12 +106,20 @@ pub struct LocalKey<T: 'static> {
 #[stable(feature = "rust1", since = "1.0.0")]
 #[allow_internal_unstable]


this attribute must be redundant since I was able to simply copy the macro into a playpen, right?

OK, it shouldn't be redundant because __thread_local_inner! is marked unstable, but something is failing to check that!

durka · 2016-06-05T02:37:33Z

BTW, RFC 1422 pub(restricted) support can be added: https://is.gd/LnAsPq

However, as long as RFC 1575 languishes, it requires the ugly reparse trick.

alexcrichton · 2016-06-05T05:41:29Z

It might be good to do a crater run just to be super sure this doesn't break anything, but otherwise this is a change in the API of the macro so I'm gonna tag with T-libs to ensure it receives discussion. There is some previous discussion about allowing attributes on thread locals but it ended up largely being inconclusive (and didn't happen despite the name of the PR and the merging of the PR).

alexcrichton · 2016-06-05T05:42:03Z

Oh and also, can you be sure to add tests which exercise this syntax as well?

durka · 2016-06-05T05:58:39Z

I'll add tests and comments in the morning.

I just read that previous discussion and it seems to me to support this
change -- with thread_local not being able to take attributes it's still
incompatible with forbidden lints, because the thread locals can't be
documented.

The change to allow semicolons and multiple declarations is just sugar, but
it matches the API of similar macros like lazy_static, plus once you have a
doc comment + newline + declaration it looks super weird (IMO) to leave out
the semicolon.
On Jun 5, 2016 01:42, "Alex Crichton" notifications@github.com wrote:

Oh and also, can you be sure to add tests which exercise this syntax as
well?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#34077 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAC3n_S3BedrZ9pWTLCQ2sSj5ryuMKgpks5qImHPgaJpZM4IuMuB
.

durka · 2016-06-06T14:35:17Z

I think this is ready for cratering cc @brson

brson · 2016-06-07T23:23:55Z

I'll start a crater run.

brson · 2016-06-07T23:27:26Z

Here are the toolchain builds:

When these are done I can run the crate builds.

brson · 2016-06-08T23:36:43Z

I've started the crate builds.

brson · 2016-06-10T02:14:13Z

@durka https://gist.github.com/brson/b15d70b39e356af286cbf404c0d07286 sorry it took so long.

durka · 2016-06-10T15:02:38Z

No regressions!

aturon · 2016-06-10T22:22:58Z

cc @rust-lang/libs Any concerns about this change? Passes crater with no regressions.

sfackler · 2016-06-10T22:24:05Z

👍

alexcrichton · 2016-06-10T23:37:32Z

src/libstd/thread/local.rs

+    );
+
+    // rule 2: handle a single private declaration
+    ($(#[$attr:meta])* static $name:ident: $t:ty = $init:expr $(;)*) => (


This'll unfortunately accept syntax like:

thread_local!(static FOO: usize = 2 ;;;;;;;;;;;;;;;;;;;;;);

Although I'm not sure how much we can do about that.

alexcrichton · 2016-06-10T23:39:03Z

I'd prefer to not accept the syntax I mentioned above, but I don't feel particularly strongly about that and would be fine landing.

brson · 2016-06-11T01:08:02Z

I don't have strong opinions about this.

durka · 2016-06-11T03:24:50Z

@alexcrichton so add a one-or-none macro matcher :)
On Jun 10, 2016 9:08 PM, "Brian Anderson" notifications@github.com wrote:

I don't have strong opinions about this.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34077 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAC3n290RR7HfYktlzGx73tBo2WGnlksks5qKgqXgaJpZM4IuMuB
.

durka · 2016-06-11T19:59:59Z

@alexcrichton fixed, at the expense of accepting thread_local!() (can be removed as well, but requires more code duplication)

alexcrichton · 2016-06-12T10:17:19Z

Ok, thanks @durka! This does mean that thread_local!(); is valid, but that seems fine to me.

alexcrichton · 2016-06-21T04:41:44Z

The libs team discussed this during triage today and the conclusion was to merge. Thanks again for the PR and the patience @durka! If you want to squash the commits I'll r+

durka · 2016-06-22T03:31:27Z

🏁

alexcrichton · 2016-06-22T20:18:19Z

Looks like the travis error is probably legit:

failures:

---- thread_local!_0 stdout ----
    <std macros>:13:1: 13:13 error: macro expansion ignores token `thread_local` and any following
<std macros>:13 thread_local ! ( $ ( $ rest ) * ) ; ) ; (
                ^~~~~~~~~~~~
<anon>:5:1: 10:2 note: caused by the macro expansion here; the usage of `thread_local!` is likely invalid in expression context
<anon>:5 thread_local! {
         ^
<std macros>:16:22: 16:25 error: expected expression, found keyword `pub`
<std macros>:16 $ ( # [ $ attr ] ) * pub static $ name : $ crate :: thread :: LocalKey < $ t >
                                     ^~~
thread 'thread_local!_0' panicked at 'Box<Any>', src/libsyntax/ext/tt/macro_rules.rs:71
note: Run with `RUST_BACKTRACE=1` for a backtrace.
thread 'thread_local!_0' panicked at 'couldn't compile the test', src/librustdoc/test.rs:287


failures:
    thread_local!_0

durka · 2016-06-22T20:59:27Z

That's what I get for not testing my doc example! Though I did test it in
the playground. Not sure how it's getting evaluated in expression context
here...
On Jun 22, 2016 16:18, "Alex Crichton" notifications@github.com wrote:

Looks like the travis error is probably legit:

failures:

---- thread_local!_0 stdout ----
:13:1: 13:13 error: macro expansion ignores token thread_local and any following
:13 thread_local ! ( $ ( $ rest ) * ) ; ) ; (
^~~~~~~~~~~~
:5:1: 10:2 note: caused by the macro expansion here; the usage of thread_local! is likely invalid in expression context
:5 thread_local! {
^
:16:22: 16:25 error: expected expression, found keyword pub
:16 $ ( # [ $ attr ] ) * pub static $ name : $ crate :: thread :: LocalKey < $ t >
^~~
thread 'thread_local!_0' panicked at 'Box', src/libsyntax/ext/tt/macro_rules.rs:71
note: Run with RUST_BACKTRACE=1 for a backtrace.
thread 'thread_local!_0' panicked at 'couldn't compile the test', src/librustdoc/test.rs:287

failures:
thread_local!_0

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34077 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAC3n5_ojM93OI_lWNC-ReptgQVCi0_Cks5qOZitgaJpZM4IuMuB
.

Allows declaring multiple statics in one macro invocation, and supports attaching attributes to the generated items.

durka · 2016-06-23T00:30:23Z

@alexcrichton OK, I think it's a bug and I filed #34418. Travis should pass now (if it fails again I'll relent and run the build locally).

alexcrichton · 2016-06-23T21:32:31Z

@bors: r+ fc28ee2

bors · 2016-06-23T22:42:44Z

⌛ Testing commit fc28ee2 with merge aeebcfd...

bors · 2016-06-23T22:48:52Z

💔 Test failed - auto-mac-64-opt

durka · 2016-06-23T23:19:31Z

Spurious failure, LLVM didn't build
On Jun 23, 2016 18:49, "bors" notifications@github.com wrote:

💔 Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/9564

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34077 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAC3n-CTE8CSahht9fj-2cr2MlxptiJKks5qOw10gaJpZM4IuMuB
.

alexcrichton · 2016-06-23T23:30:45Z

@bors: retry

On Thu, Jun 23, 2016 at 4:19 PM, Alex Burka notifications@github.com
wrote:

Spurious failure, LLVM didn't build
On Jun 23, 2016 18:49, "bors" notifications@github.com wrote:

💔 Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/9564

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34077 (comment),
or mute
the thread
<
https://github.com/notifications/unsubscribe/AAC3n-CTE8CSahht9fj-2cr2MlxptiJKks5qOw10gaJpZM4IuMuB

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34077 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95JDiDvMYgepuAudiLs3OyPeVeuS8ks5qOxSHgaJpZM4IuMuB
.

bors · 2016-06-23T23:48:51Z

⌛ Testing commit fc28ee2 with merge c7e8ee9...

bors · 2016-06-23T23:54:26Z

💔 Test failed - auto-mac-cross-ios-opt

alexcrichton · 2016-06-23T23:56:43Z

@bors: retry

On Thu, Jun 23, 2016 at 4:54 PM, bors notifications@github.com wrote:

💔 Test failed - auto-mac-cross-ios-opt
http://buildbot.rust-lang.org/builders/auto-mac-cross-ios-opt/builds/714

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34077 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95HLcwxtRlTLEYxtJCNvTFCSkCL0fks5qOxy1gaJpZM4IuMuB
.

bors · 2016-06-24T02:22:51Z

⌛ Testing commit fc28ee2 with merge 4d9faf3...

@frankmcsherry

upgrade thread_local! invocation syntax Allows declaring multiple statics in one macro invocation, and supports attaching attributes to the generated items. In particular, `#![forbid(missing_docs, unused)]` is now tenable on a crate/module containing thread locals. For an example see [here](https://is.gd/aVFZZF). This change is fully backwards compatible as far as I can tell. cc @frankmcsherry

bors · 2016-06-24T06:33:30Z

rust-highfive assigned aturon Jun 4, 2016

durka reviewed Jun 4, 2016
View reviewed changes

alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 5, 2016

alexcrichton reviewed Jun 10, 2016
View reviewed changes

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 11, 2016

alexcrichton removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 21, 2016

durka force-pushed the patch-23 branch from 524f35d to 9422a6f Compare June 22, 2016 03:29

durka force-pushed the patch-23 branch from 9422a6f to ca53bc1 Compare June 22, 2016 03:39

upgrade thread_local! invocation syntax

fc28ee2

Allows declaring multiple statics in one macro invocation, and supports attaching attributes to the generated items.

durka force-pushed the patch-23 branch from ca53bc1 to fc28ee2 Compare June 23, 2016 00:29

bors merged commit fc28ee2 into rust-lang:master Jun 24, 2016

		@@ -106,12 +106,20 @@ pub struct LocalKey<T: 'static> {
		#[stable(feature = "rust1", since = "1.0.0")]
		#[allow_internal_unstable]

upgrade thread_local! invocation syntax #34077

upgrade thread_local! invocation syntax #34077

Conversation

durka commented Jun 4, 2016

rust-highfive commented Jun 4, 2016

durka Jun 4, 2016

Choose a reason for hiding this comment

durka Jun 4, 2016

Choose a reason for hiding this comment

durka commented Jun 5, 2016

alexcrichton commented Jun 5, 2016

alexcrichton commented Jun 5, 2016

durka commented Jun 5, 2016

durka commented Jun 6, 2016

brson commented Jun 7, 2016

brson commented Jun 7, 2016

brson commented Jun 8, 2016

brson commented Jun 10, 2016

durka commented Jun 10, 2016

aturon commented Jun 10, 2016

sfackler commented Jun 10, 2016

alexcrichton Jun 10, 2016

Choose a reason for hiding this comment

alexcrichton commented Jun 10, 2016

brson commented Jun 11, 2016

durka commented Jun 11, 2016

durka commented Jun 11, 2016

alexcrichton commented Jun 12, 2016

alexcrichton commented Jun 21, 2016

durka commented Jun 22, 2016

alexcrichton commented Jun 22, 2016

durka commented Jun 22, 2016

durka commented Jun 23, 2016

alexcrichton commented Jun 23, 2016

bors commented Jun 23, 2016

bors commented Jun 23, 2016

durka commented Jun 23, 2016

alexcrichton commented Jun 23, 2016

bors commented Jun 23, 2016

bors commented Jun 23, 2016

alexcrichton commented Jun 23, 2016

bors commented Jun 24, 2016

bors commented Jun 24, 2016