Simplify profiler impl (bubble up Option) #4148
Conversation
|
Force-pushed due to some rustup connection problem on CI: |
crates/ra_prof/src/hprof.rs
Outdated
| let label = if enabled { Some(label) } else { None }; | ||
| Profiler { label, detail: None } | ||
| if PROFILING_ENABLED.load(Ordering::Relaxed) { | ||
| PROFILE_STACK.with(|stack| stack.borrow_mut().push(label)); |
There was a problem hiding this comment.
This changes semantics, it’ll return non trivial profiler where the old code would use None label
There was a problem hiding this comment.
What do you mean by non-trivial profiler, it is just a newtype of Option<Impl>
There was a problem hiding this comment.
Reviewing from my iPad, so might be wrong, but it still seems to me that the refactoring changes behavior. && was meaningful.
There was a problem hiding this comment.
You are right, I mistakenly thought that the boolean value returned from push is unused, already fixed it.
Regarding the non-trivilaity, the rustc is smart enough not to increase the size of the profiler and use the inner string reference non-nullability to store the discriminant.
|
@matklad, this one is easy, do we merge it? |
|
bors r+ |
|
Might need a rebase. |
|
Build succeeded: |
No description provided.