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

Add macros to get the values of configuration flags #1258

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
8 participants
@dylanmckay
Copy link

dylanmckay commented Aug 16, 2015

This RFC adds four macros which take a compile-time configuration flag (e.g. target_os) and evaluate to either a compile time integer literal, float literal, string literal, or an identifier.

There is an implementation of it in Rust PR 27855 (although it is missing cfg_ident!).

Rendered

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 16, 2015

Thanks @dylanmckay! Would it be possible to scale this back to solve the problem at hand? This is adding quite a bit of surface area which looks like it may not all be necessary to solve the motivation at hand. For example, is cfg_float! necessary? It seems like while there may be a theoretical application there doesn't seem to be a strong motivation for it in practice.

I also think that the cfg_ident! macro may not work out, your example of:

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 16, 2015

Guh sorry for the early comment:

your example of:

pub mod cfg_ident!(target_os);

Have you tested this? We can't, for example, do fn concat_idents!(foo, bar)() {} so I would be surprised if this worked. Along those lines this may be able to cut the cfg_ident! macro as well.

This may also want to consider what to do with the case that a #[cfg] key isn't set instead of just generating a compiler error. It may be more useful in some situations to return an Option (e.g. env! vs option_env!).

Finally, the naming of cfg_int! is a little odd there's no "int" type in the language, and it may be best to just say it returns an i32 perhaps?

@dylanmckay

This comment has been minimized.

Copy link
Author

dylanmckay commented Aug 17, 2015

Hello @alexcrichton!

Thanks the the feedback - you raise very valid points. I originally planned on adding a single cfg_int!() macro. After I implemented that, I realised how easy it would be to add the others so I went along and did that.

For example, is cfg_float! necessary?

cfg_float! did seem to have a very small (if existent) usecase. I included it so that it could be discussed in the RFC (plus it was very easy to implement). I agree with you - this macro should be pruned from the RFC. I'll update it.

With regards to cfg_ident!(), I did not realise that concat_idents!() was so limited. Indeed, today cfg_ident!() would be just as crippled, leading it to be essentially useless. I will remove this from the RFC as well.

Finally, the naming of cfg_int! is a little odd there's no "int" type in the language, and it may be best to just say it returns an i32 perhaps?

I would prefer the caller of the macro to be able to use the macro and have it expand into an unsuffixed integer literal - that way the type of the integer can be inferred automatically.

This has several benefits:

  • If the flag value overflows a u32 - what happens? The only option would be for the compiler to crash - truncating or ignoring the error would lead to incorrect results
  • If type-level integers were implemented, what exactly would let v: Vector3<cfg_int!(dimensions)> mean? Vector3<3u32> might not make much sense - suffixing a type-level integer may not inter operate with a future type level integer proposal.
  • Users of the macro can have the macro expand into an arbitrary sized integer - if cfg_int!(foo) as usize < my_var { /* ... */ }, removing unnecessary and redundant casts.

On that note, perhaps cfg_int!() is not an appropriate name. It suggests that it expands into a value of type int - which does not exist. What do you think about modifying the proposal so that it becomes cfg_integer!() instead. This removes any connotation about the type of the expanded literal.

This may also want to consider what to do with the case that a #[cfg] key isn't set instead
of just generating a compiler error. It may be more useful in some situations to return an
Option (e.g. env! vs option_env!).

This is a good idea. We could add a option_cfg_str!() macro and an option_cfg_int!() macro. It is, however, very important to maintain a distinction between the macros which return literals vs. the macros which return Options of values. I'll add the option variants into the RFC.

Dylan McKay
Revised the RFC
`cfg_int` -> `cfg_integer!`

Removed `cfg_float!` and `cfg_ident!`.

Added `option_cfg_integer!` and `option_cfg_str!`
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Aug 17, 2015

I am very much for the alternative about cfg_value!

Wild suggestion: cfg_value!(foo, Option<i32>) could be used instead of a cfg_optional_value! macro.

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Aug 20, 2015

There's an unfortunate hitch here: cfgs aren't unique, e.g. cargo will add multiple --cfg feature="..." based on the features specified, so, cargo build --features "foo bar baz" will cause all the following functions to exist:

#[cfg(feature = "foo")]
fn foo() {}
#[cfg(feature = "bar")]
fn bar() {}
#[cfg(feature = "baz")]
fn baz() {}
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 20, 2015

Is there any way to have fewer macros here?

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Aug 20, 2015

to expand upon @nikomatsakis 's note, there was some informal discussion of maybe employing some sort of duck-typing in order to reduce the number of macros.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Nov 19, 2015

@dylanmckay have you given any further thought on ways to reduce the number of macros here? I did see the suggestion in the alternatives section; but I'm curious if you've had further thoughts on whether we can do something more aggressive...

@dylanmckay

This comment has been minimized.

Copy link
Author

dylanmckay commented Nov 20, 2015

Ideally, I think it would be best to expand the macro into a special type which can be coerced into either an integer or a string, depending on whatever type inference decides, and the value of the flag.

This would allow us to have a cfg_value! and cfg_optional_value! macros.

Anything other than that is simply working around the fact that macros aren't typed.

You should be able to do

if cfg_value!(target_os) == "windows" ||
    cfg_value!(custom_feature) == 12 { .. }

I'm not sure how well this would play with the inference engine.

@pnkfelix What are your thoughts? Have a single macro for several types, or have both integer and string variants?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 11, 2016

🔔 This RFC is now entering its week-long final comment period 🔔


My own personal opinion here is that it may be a bit premature to add these macros just yet, but we may want something along these lines eventually! The design here I think may need some improvement in terms of ergonomics and returned types.

@poelzi

This comment has been minimized.

Copy link

poelzi commented Feb 16, 2016

Wild suggestion: cfg_value!(foo, Option) could be used instead of a cfg_optional_value! macro

This would look much better, +1 for cfg_value!(foo, Option)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 18, 2016

The libs team discussed this during triage yesterday and the conclusion was to close this RFC for now. We're specifically still worried about what these macros would do in the face of a cfg being defined multiple times (as @huonw mentioned), and it seems like the names/semantics of the macros here may want to have more consensus before moving forward.

The next steps here would probably be to start a thread on the internals or users forums to see what others' use cases might look like and get feedback on the design here. There's unfortunately a number of downsides to #[cfg] as-is today and this also may not be the most prominent on others' minds in terms of what to fix next!

Thanks again for the RFC though @dylanmckay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.