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

Replace `Option<&str>` with `Option<Enum>` in `cpp_link_stdlib` / `cpp_set_stdlib` #184

Closed
brson opened this Issue Jul 13, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@brson
Copy link
Contributor

brson commented Jul 13, 2017

I'm not sure this issue is correct actually.

In our review we were talking like this was an enum representing the C++ standard, but it is actually an option to control the name of the C++ standard library, e.g. "c++", "stdc++". Is this really a closed set that we can enumerate?

Needs further discussion before action. cc @alexcrichton @sfackler

@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Jul 14, 2017

It's true yeah that the set of libraries here is probably small, although I'd be wary of trying to use an exhaustive enum as this'll probably expand over time. I wonder if an enum directly may not buy much here? I'm not sure this is really a cross-toolchain option in the sense that there's a cross-platform intention you can specify and gcc-rs figures it out. Rather we may want to just have some doc examples of the possible values and whatnot here and document various strings rather than codify them in an enum.

@dtolnay

This comment has been minimized.

Copy link

dtolnay commented Jul 31, 2017

Symbols and capitalization make it awkward to use anything but a &str.

// enum
cpp_set_stdlib(gcc::Stdlib::StdCPlusPlus) // or StdCxx

// const
cpp_set_stdlib(gcc::STD_C_PLUS_PLUS) // STDCXX?

// raw identifier?? https://internals.rust-lang.org/t/pre-rfc-raw-identifiers/5502
cpp_set_stdlib(gcc::r#stdc++#)

// macro, able to catch typo at compile time but seems unnecessary
cpp_set_stdlib(stdlib!("stdc++"))

Rather we may want to just have some doc examples of the possible values and whatnot here and document various strings rather than codify them in an enum.

I agree.

@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Jul 31, 2017

Ok, let's stick with Option<&str> here

@dtolnay

This comment has been minimized.

Copy link

dtolnay commented Jul 31, 2017

I filed #223 to follow up with documentation.

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.