gh-bitflags/bitflags

Watch

Fork 0

Support for additional access specifiers #126

Closed

GabrielMajeri wants to merge 6 commits from pub-restricted into master

GabrielMajeri commented

2017-10-11 11:31:34 -05:00

(Migrated from github.com)

This pull request adds support for new access specifiers like pub (crate) or pub (in some_module).

This is currently a nightly only feature, so this pull request should not currently be merged.
It breaks all doctests and integration tests, since they'd need to manually enable #![feature(macro_vis_matcher)].

Fixes #72 (once merged).

This pull request adds support for new access specifiers like `pub (crate)` or `pub (in some_module)`. This is currently a `nightly` only feature, so this pull request should not currently be merged. It breaks all doctests and integration tests, since they'd need to manually enable `#![feature(macro_vis_matcher)]`. Fixes #72 (once merged).

KodrAus commented

2017-10-11 16:02:36 -05:00

(Migrated from github.com)

Nice work! Thanks @GabrielMajeri. I'm not sure if we've got an idea of when macro_vis_matcher should stabilise yet, but hopefully there shouldn't be much churn here so I think it's safe to keep this around for a bit.

Also worth noting that once it does stabilise and we do merge then the minimum rustc version will be bumped.

Nice work! Thanks @GabrielMajeri. I'm not sure if we've got an idea of when `macro_vis_matcher` should stabilise yet, but hopefully there shouldn't be much churn here so I think it's safe to keep this around for a bit. Also worth noting that once it does stabilise and we do merge then the minimum `rustc` version will be bumped.

tamird commented

2017-10-17 08:55:04 -05:00

(Migrated from github.com)

This can go in behind a cargo feature, right?

GabrielMajeri commented

2017-10-17 09:57:37 -05:00

(Migrated from github.com)

@tamird yes it can, I've now placed it behind the visibility Cargo feature.

@tamird yes it can, I've now placed it behind the `visibility` Cargo feature.

KodrAus commented

2017-10-17 18:01:16 -05:00

(Migrated from github.com)

Hmm, I'm not personally sold on the need for a cargo feature, at least not long-term. I get that bumping the minimum rustc version is potentially breaking, especially if we bump it to the latest stable right away. But since pub(restricted) is a fundamental language feature it seems weird to need a cargo feature to support a language feature in another language feature. So if we did want one now to minimise breakage then I'd be happy if we had a plan to make it the status-quo down the track.

Do you have any thoughts @alexcrichton?

Hmm, I'm not personally sold on the need for a `cargo` feature, at least not long-term. I get that bumping the minimum `rustc` version is potentially breaking, especially if we bump it to the latest stable right away. But since `pub(restricted)` is a fundamental language feature it seems weird to need a cargo feature to support a language feature in another language feature. So if we did want one now to minimise breakage then I'd be happy if we had a plan to make it the status-quo down the track. Do you have any thoughts @alexcrichton?

tamird commented

2017-10-17 18:10:34 -05:00

(Migrated from github.com)

I don't follow, @KodrAus. Using a cargo feature allows this to merge before the necessary bits are stabilized upstream. Bumping the minimum rust version is a separate discussion - there's no reason to hold this up on that decision.

👍 1

KodrAus commented

2017-10-17 18:46:49 -05:00

(Migrated from github.com)

Yep, sorry @tamird I was getting ahead of myself 😅 Feel free to ignore all of that until vis stabilises.

Yep, sorry @tamird I was getting ahead of myself 😅 Feel free to ignore all of that until `vis` stabilises.

alexcrichton commented

2017-10-17 19:41:24 -05:00

(Migrated from github.com)

Oh I figured we'd just add support when this is in stable rust, but I don't think it is?

KodrAus commented

2017-10-18 07:14:39 -05:00

(Migrated from github.com)

Yeh it looks like it's still unstable.

I was originally thinking we shouldn't release with a cargo feature because then we'd have to maintain a fork of the macro, and people shouldn't need to use a feature to get pub(restricted) once it's stable.

But I think we could use a feature, and then when it does stabilise we could just make that feature a no-op, since the :vis macro version is essentially more general.

I guess the alternative is not use a feature and let this PR sit here waiting until it can work on stable, but I don't know when that would be.

Yeh it looks like it's still unstable. I was originally thinking we shouldn't release with a cargo feature because then we'd have to maintain a fork of the macro, and people shouldn't need to use a feature to get `pub(restricted)` once it's stable. But I think we could use a feature, and then when it does stabilise we could just make that feature a no-op, since the `:vis` macro version is essentially more general. I guess the alternative is not use a feature and let this PR sit here waiting until it can work on stable, but I don't know when that would be.

alexcrichton commented

2017-10-18 09:05:50 -05:00

(Migrated from github.com)

That's true yeah, I'd also be ok landing it behind an off-by-default feature.

dtolnay (Migrated from github.com) reviewed

2017-10-22 15:05:36 -05:00

dtolnay (Migrated from github.com) left a comment

I would prefer to do this in a way that immediately works on stable. Would something like this be possible?

/*
#![feature(macro_vis_matcher)]

macro_rules! m {
    ($vis:vis const $n:ident) => {
        $vis const $n: i32 = 0;
    };
}
*/

macro_rules! m {
    (const $n:ident) => {
        __impl_m!(() const $n);
    };
    (pub const $n:ident) => {
        __impl_m!((pub) const $n);
    };
    (pub ($($vis:tt)+) const $n:ident) => {
        __impl_m!((pub ($($vis)+)) const $n);
    };
}

macro_rules! __impl_m {
    (($($vis:tt)*) const $n:ident) => {
        $($vis)* const $n: i32 = 0;
    };
}

m!(const PRIV);
m!(pub(crate) const CRATE);
m!(pub const PUB);

fn main() {
    println!("{} {} {}", PRIV, CRATE, PUB);
}

I would prefer to do this in a way that immediately works on stable. Would something like this be possible? ```rust /* #![feature(macro_vis_matcher)] macro_rules! m { ($vis:vis const $n:ident) => { $vis const $n: i32 = 0; }; } */ macro_rules! m { (const $n:ident) => { __impl_m!(() const $n); }; (pub const $n:ident) => { __impl_m!((pub) const $n); }; (pub ($($vis:tt)+) const $n:ident) => { __impl_m!((pub ($($vis)+)) const $n); }; } macro_rules! __impl_m { (($($vis:tt)*) const $n:ident) => { $($vis)* const $n: i32 = 0; }; } m!(const PRIV); m!(pub(crate) const CRATE); m!(pub const PUB); fn main() { println!("{} {} {}", PRIV, CRATE, PUB); } ```

KodrAus commented

2017-11-07 19:01:54 -06:00

(Migrated from github.com)

Neat! What do you think @GabrielMajeri?

GabrielMajeri commented

2017-11-07 23:17:44 -06:00

(Migrated from github.com)

@KodrAus I have created a new pull request for that (#135), and will keep this updated for when vis stabilizes.

@KodrAus I have created a new pull request for that (#135), and will keep this updated for when `vis` stabilizes.

dtolnay commented

2017-11-12 09:48:59 -06:00

(Migrated from github.com)

Superseded by #135.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

No reviewers

No labels

bug

good first issue

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-bitflags/bitflags!126

No description provided.

Rows
Columns