Consistently pass Dispatch as owned value #1051
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As an alternative to tokio-rs#1045, this PR switches `set_default` and `with_default` to take an owned `Dispatch` instead of a ref. See [comment here](tokio-rs#1045 (comment)) for motivation. Closes tokio-rs#455 Part of tokio-rs#922
dvdplm
commented
Oct 19, 2020
dvdplm
changed the title
…spatch-as-owned-tokio-rs#455 * upstream/master: (34 commits) subscriber: remove TraceLogger (tokio-rs#1052) subscriber: make Registry::enter/exit much faster (tokio-rs#1058) chore(deps): update env_logger requirement from 0.7 to 0.8 (tokio-rs#1050) chore: fix tracing-macros::dbg (tokio-rs#1054) chore(deps): update pin-project requirement from 0.4 to 1.0 (tokio-rs#1038) chore: remove duplicated section from tracing/README.md (tokio-rs#1046) opentelemetry: prepare for 0.8.0 release (tokio-rs#1036) docs: add favicon for extra pretty docs (tokio-rs#1033) subscriber: fix `reload` ergonomics (tokio-rs#1035) chore(deps): update crossbeam-channel requirement from 0.4.2 to 0.5.0 (tokio-rs#1031) opentelemetry: Assign default ids if missing (tokio-rs#1027) chore: remove deprecated add-path from CI (tokio-rs#1026) attributes: fix `#[instrument(err)]` in case of early returns (tokio-rs#1006) core: remove mandatory liballoc dependency with no-std (tokio-rs#1017) chore(deps): update cfg-if requirement from 0.1.10 to 1.0.0 (tokio-rs#1023) subscriber: warn if trying to enable a statically disabled level (tokio-rs#990) subscriber: use macros for module declarations (tokio-rs#1009) chore: remove `stdlib.rs` (tokio-rs#1008) core: fix linked list tests reusing `Registration`s (tokio-rs#1016) subscriber: support dash in target names (tokio-rs#1012) ...
…spatch-as-owned-tokio-rs#455 * upstream/master: subscriber: update sharded-slab to 0.1, pool hashmap allocations (tokio-rs#1062) subscriber: remove deprecated type, structs, and methods tokio-rs#1030 core: rename Subscriber to Collect (tokio-rs#1015) chore: fix rustdoc warning in tracing-subscriber (tokio-rs#1061)
|
It looks like |
hawkw
requested changes
Oct 28, 2020
There was a problem hiding this comment.
Thanks for working on this. I think this change (passing the Dispatch/Subscriber by value in all cases, rather than by ref) is the right approach.
However, I noticed a couple things we'll need to change before we can merge this:
- I think this branch needs to be rebased onto the latest master, since some modules were renamed on master. Also, there are a few merge conflicts that need to be resolved.
- I think this changes the behavior of the
tracing_subscriber::registrytests slightly, so that they will always pass, even if the underlying behavior being tested changes. We should fix those tests by ensuring that theDispatchis always dropped at the end of the test, not whenwith_defaultfinishes, by cloning it when callingwith_default.
Thanks!
…nt to clone the Dispatch
|
CI failure seems spurious. |
|
@hawkw Is there anything else needed here? :) |
|
Maybe this can still be merged, will need to confirm. |
bryangarza
moved this from Pending direction review
to If we rebase and re-review, it could probably be merged
in PR finish line
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As an alternative to #1045, this PR switches
set_defaultandwith_defaultto take an ownedDispatchinstead of a ref.See comment here for motivation.
Closes #455
Part of #922