Skip to content

Cleanup: replace cfg-if with our macros #361

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

Merged
4 commits merged into from Oct 17, 2019
Merged

Cleanup: replace cfg-if with our macros #361

4 commits merged into from Oct 17, 2019

Conversation

ghost
Copy link

@ghost ghost commented Oct 17, 2019

This is a negative diff that only reshuffles code and cleans it up.

I added a few convenience macros that replace all uses of cfg_if! with new macros unstable!, unix!, windows!, docs!, and not_docs!.

@yoshuawuyts
Copy link
Contributor

@stjepang maybe prefixing the macro names with cfg_ would make it clearer what's going on for people not familiar with them? E.g. cfg_unix!

@ghost
Copy link
Author

ghost commented Oct 17, 2019

@yoshuawuyts Sounds good! I added the cfg_ prefix to the new macros.

@ghost ghost requested a review from taiki-e October 17, 2019 12:30
src/utils.rs Outdated
@@ -20,6 +20,69 @@ pub fn abort_on_panic<T>(f: impl FnOnce() -> T) -> T {
t
}

/// Declares unstable items.
#[doc(hidden)]
#[macro_export]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these macros exported?

I think it is enough to use #[macro_use] for the utils module if these macros are only used in this crate.

#[macro_use]
mod utils;

Copy link
Author

Choose a reason for hiding this comment

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

This doesn't work - we have to keep #[macro_export] or else the macro won't be visible from other modules. At least that's my understanding.

Do you know how to make this work without #[macro_export]?

Copy link
Contributor

@taiki-e taiki-e Oct 17, 2019

Choose a reason for hiding this comment

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

I made it work by doing the following. (see https://github.com/stjepang/async-std/pull/1)

  • Apply #[macro_use] to the module where the macro is defined.
  • Remove crate:: from all internal macro calls.
  • Remove #[macro_export] from the internal macro.

@ghost
Copy link
Author

ghost commented Oct 17, 2019

@taiki-e Thanks for removing #[macro_export]! If this looks good now, feel free to merge.

@ghost ghost merged commit ec23632 into async-rs:master Oct 17, 2019
@ghost ghost deleted the cleanup-macros branch October 17, 2019 17:17
This pull request was closed.
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.

2 participants