Tidy ups #549
Tidy ups #549
Conversation
|
bors try |
tryBuild succeeded: |
yktrace/src/lib.rs
Outdated
| @@ -25,7 +25,7 @@ thread_local! { | |||
| // | |||
| // We hide the `ThreadTracer` in a thread local (rather than returning it to the consumer of | |||
| // yk). This ensures that the `ThreadTracer` itself cannot appear in traces. | |||
| pub static THREAD_TRACER: RefCell<Option<ThreadTracer>> = const { RefCell::new(None) }; | |||
| pub static THREAD_TRACER: RefCell<Option<ThreadTracer>> = RefCell::new(None); | |||
There was a problem hiding this comment.
Creating a RefCell (calling RefCell::new()) is const, so this may be right? IIRC, the const isn't talking about whether the data is mutable, but whether the call can be evaluated at compile time(?).
(I didn't know you had to explcitiely mark it const though)
There was a problem hiding this comment.
Clippy complained obliquely about this const: I'm not sure what it means for a RefCell to be const if it's instantiated multiple times in multiple threads. Unless we know why it's here, I think that at most const is an optimisation (though possibly not even that in this case), and possible it's incorrect.
There was a problem hiding this comment.
I'm not sure what it means for a RefCell to be const if it's instantiated multiple times in multiple threads.
Should be fine. It's a thread local.
There was a problem hiding this comment.
What happens if const here means "share one RefCell across multiple threads"? I don't think we know precisely what const means, and there's a small chance it interacts incorrectly with thread locals.
There was a problem hiding this comment.
That'd be bonkers if that's what happened, but I'll find the answer.
There was a problem hiding this comment.
I can't find it spelled out anywhere, but some part of tokio has applied the same optimisation recently (it's still under PR, but I think the intent is clear).
FWIW, tracking issue for the feature (no mention of surprising semantic changes).
I just don't see why thread_local! {const ...} would make something not a thread local :P
There was a problem hiding this comment.
Maybe try running clippy locally and see what you think of its complaint here?
There was a problem hiding this comment.
I'm now questioning clippy. Is it is conflating const data with a const block?!
https://rust-lang.github.io/rust-clippy/master/index.html#declare_interior_mutable_const
Look at the examples. This is different to what is going on here!
And there it is. This is a clippy bug!
There was a problem hiding this comment.
Good find. So I should delete the commit that makes this change, I think?
There was a problem hiding this comment.
I think so, yes. May as well force push that now.
|
Force pushed. |
|
bors r+ |
|
Build succeeded: |
Some random tidy-ups, partly found by Clippy.
The only tricky one is e0aceed which @vext01 might want to take a quick peek at.