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

Use macros for module declarations #1009

Merged
merged 4 commits into from
Oct 6, 2020

Conversation

k-nasa
Copy link
Contributor

@k-nasa k-nasa commented Oct 2, 2020

Motivation

This is code refactoring. Currently, there is a lot of code that declares modules as follows:

#[cfg(feature = "registry")]
#[cfg_attr(docsrs, doc(cfg(feature = "registry")))]
pub use registry::Registry;

#[cfg(feature = "registry")]
#[cfg_attr(docsrs, doc(cfg(feature = "registry")))]
pub fn registry() -> Registry {
    Registry::default()
}

It is verbose to write a module declaration multiple times using the feature attribute. Also, if the definition for the same feature spans multiple places, it will be a little harder to read the code.

Solution

You can combine features attributes in one place by writing as follows. It also eliminates the need to write the same code over and over again.

cfg_registry! {
    pub use registry::Registry;

    pub fn registry() -> Registry {
        Registry::default()
    }
}

f this code is accepted, we will extend the declaration of the module using macros to other files as well as tracing-subscriber/lib.rs.

@k-nasa k-nasa requested review from hawkw and a team as code owners October 2, 2020 03:16
Comment on lines 18 to 26
macro_rules! cfg_fmt {
($($item:item)*) => {
$(
#[cfg(feature = "fmt")]
#[cfg_attr(docsrs, doc(cfg(feature = "fmt")))]
$item
)*
}
}
Copy link
Contributor

@jyn514 jyn514 Oct 2, 2020

Choose a reason for hiding this comment

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

You could make this one macro instead of two by taking the feature as a parameter, something like this:

Suggested change
macro_rules! cfg_fmt {
($($item:item)*) => {
$(
#[cfg(feature = "fmt")]
#[cfg_attr(docsrs, doc(cfg(feature = "fmt")))]
$item
)*
}
}
macro_rules! cfg_fmt {
(#[feature = $($name: lit) ] $($item:item)*) => {
$(
#[cfg(feature = $name)]
#[cfg_attr(docsrs, doc(cfg(feature = $name)))]
$item
)*
}
}

Copy link
Contributor Author

@k-nasa k-nasa Oct 2, 2020

Choose a reason for hiding this comment

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

I fixed it as follows.
9b4f0e3

@hawkw
Copy link
Member

hawkw commented Oct 2, 2020

I know that in Tokio we've had some issues in the past with some editors' code completion and other features not working when definitions are hidden inside macro expansion. Do you know if that issue is still widespread enough to be a concern about merging this?

@k-nasa
Copy link
Contributor Author

k-nasa commented Oct 4, 2020

@halzy I didn't know this issue... I didn't have such issue in my editor. I don't think this change is acceptable if the code completion doesn't work.
However, it felt like a kind of trade-off because it made the code more readable.

@alce
Copy link
Member

alce commented Oct 4, 2020

fwiw, this is not a problem in Intellij anymore

@davidbarsky
Copy link
Member

I know that in Tokio we've had some issues in the past with some editors' code completion and other features not working when definitions are hidden inside macro expansion. Do you know if that issue is still widespread enough to be a concern about merging this?

This has been resolved in IntelliJ Rust 0.3 a few months ago. rust-analyzer also supports this sort of macro expansion.

However, I don't believe RLS can expand these macros. I'm personally okay with directing folks to use rust-analyzer, but I know we're going to get inquiries about this.

@hawkw
Copy link
Member

hawkw commented Oct 5, 2020

If this is no longer an issue for most widely-used IDEs/editors, I'm good with making this change. I don't have a sense for how many people are still using RLS rather than rust-analyzer, but since RLS is likely to be deprecated in favor of rust-analyzer, I don't think it's super important to support it?

@davidbarsky
Copy link
Member

Yup, I think telling folks to use rust-analyzer instead of RLS is fine.

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.

Okay, since it sounds like this shouldn't break most of the commonly used IDEs and editors, I'm good with this change.

I think there are at least a few more places in tracing-subscriber where we could also use this macro, like here:

#[cfg(feature = "env-filter")]
mod env;
mod level;
pub use self::level::{LevelFilter, ParseError as LevelParseError};
#[cfg(feature = "env-filter")]
#[cfg_attr(docsrs, doc(cfg(feature = "env-filter")))]
pub use self::env::*;

and here:
#[cfg(feature = "json")]
mod json;
use fmt::{Debug, Display};
#[cfg(feature = "json")]
#[cfg_attr(docsrs, doc(cfg(feature = "json")))]
pub use json::*;

Let's change those (and any other places where it might be applicable outside lib.rs) as well?

I would also be open to making the same change in other crates that have a lot of feature flagged code, but it would be fine to do that in separate PRs.

tracing-subscriber/src/macros.rs Outdated Show resolved Hide resolved
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@k-nasa
Copy link
Contributor Author

k-nasa commented Oct 6, 2020

I'll open another PR for change other than lib.rs! It will take some time.

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.

okay, looks good to me --- looking forwards to followups!

@hawkw hawkw merged commit cb1dd95 into tokio-rs:master Oct 6, 2020
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 7, 2020
* upstream/master:
  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)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003)
  tracing: make `Entered` `!Send` (tokio-rs#1001)
  chore: fix nightly clippy warnings (tokio-rs#991)
  chore: bump all crate versions (tokio-rs#998)
  macros: fix the `tracing-macros` crate not compiling (tokio-rs#1000)
  tracing: prepare to release 0.1.21 (tokio-rs#997)
  core: prepare to release 0.1.17 (tokio-rs#996)
  subscriber: make `PartialOrd` & `Ord` impls more correct (tokio-rs#995)
  core, tracing: don't inline dispatcher::get_default (tokio-rs#994)
  core: make `Level` and `LevelFilter` `Copy` (tokio-rs#992)
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 16, 2020
…spatch-as-ref-tokio-rs#455

* upstream/master: (28 commits)
  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)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  tracing: remove `Into<Option<Id>>` impl for `Span` (tokio-rs#1003)
  tracing: make `Entered` `!Send` (tokio-rs#1001)
  ...
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 19, 2020
…cros

* upstream/master: (30 commits)
  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)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  serde: allow tracing-serde to work on no_std. (tokio-rs#960)
  ...
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 19, 2020
…gger

* upstream/master: (31 commits)
  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)
  docs: switch to intra-doc links in tracing-core (tokio-rs#1010)
  tracing-opentelemetry: implement additional record types (bool, i64, u64) (tokio-rs#1007)
  core: add intrusive linked list for callsite registry (tokio-rs#988)
  ...
dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 22, 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)
  ...
@k-nasa k-nasa deleted the use_macro_for_module_declarations branch October 31, 2020 07:26
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.

5 participants