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

Consistently pass Dispatch as owned value #1051

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Oct 19, 2020

As an alternative to #1045, this PR switches set_default and with_default to take an owned Dispatch instead of a ref.

See comment here for motivation.

Closes #455
Part of #922

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 dvdplm requested review from hawkw and a team as code owners October 19, 2020 08:25
@dvdplm dvdplm changed the title Pass Dispatch as owned value Consistently pass Dispatch as owned value Oct 19, 2020
…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)
@hawkw
Copy link
Member

hawkw commented Oct 28, 2020

It looks like cargo check is failing on this branch because there are merge conflicts which haven't been resolved. Mind rebasing the latest master and fixing that?

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. I think this changes the behavior of the tracing_subscriber::registry tests slightly, so that they will always pass, even if the underlying behavior being tested changes. We should fix those tests by ensuring that the Dispatch is always dropped at the end of the test, not when with_default finishes, by cloning it when calling with_default.

Thanks!

tracing-subscriber/src/registry/sharded.rs Show resolved Hide resolved
tracing-subscriber/src/registry/sharded.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/registry/sharded.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/registry/sharded.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/registry/sharded.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm requested a review from hawkw November 3, 2020 13:04
@dvdplm
Copy link
Contributor Author

dvdplm commented Nov 3, 2020

CI failure seems spurious.

@dvdplm
Copy link
Contributor Author

dvdplm commented Dec 14, 2020

@hawkw Is there anything else needed here? :)

@bryangarza
Copy link
Member

Maybe this can still be merged, will need to confirm.

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.

Consistency between various *default methods
3 participants