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

Improvements to --print cfg #31671

Merged
merged 3 commits into from
Mar 4, 2016
Merged

Improvements to --print cfg #31671

merged 3 commits into from
Mar 4, 2016

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented Feb 15, 2016

Show cfg as possible argument to --print and make it so that --print cfg also outputs the target_features.

Should I also extend src/test/run-make/print-cfg/Makefile to check that target_features are actually printed?

@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Thanks! I specifically didn't include target_feature, however, as it's an explicitly unstable feature of the compiler. If these were only printed out on the nightly compiler that seems fine to me, however.

To me, though, the bug with us not querying these values from LLVM is also somewhat of a blocker as the information here is likely false.

@alexcrichton alexcrichton assigned alexcrichton and unassigned arielb1 Feb 15, 2016
@ranma42
Copy link
Contributor Author

ranma42 commented Feb 15, 2016

I am already working on a fix for #31662 that extracts the feature set from LLVM. I will resume working on this after that.
Regarding nightly, would it be ok if the cfg created by config::build_configuration included the target features only in nightly or should it include them regardless and only show that information in nightly?
AFAICT it should make no difference, as target_features should not be accessible in stable/beta.

@alexcrichton
Copy link
Member

I think it'd be fine to just not fill it in, but it would degrade the quality of the error message if you tried to use it on the stable/beta channels. I think that's fine in terms of sharing implementations, however.

@bors
Copy link
Contributor

bors commented Feb 25, 2016

☔ The latest upstream changes (presumably #31882) made this pull request unmergeable. Please resolve the merge conflicts.

ranma42 added 2 commits March 2, 2016 21:58
The `--print` flag was extended in rust-lang#31278 to accept `cfg`, but the
usage message was not updated.
The configuration returned by `config::build_configuration` needs to
be modified with `target_features::add_configuration` in order to also
contain the target features. This is already done for the
configuration used when compiling and when creating the documentation,
but was missing in the `cfg` printing code.
@ranma42
Copy link
Contributor Author

ranma42 commented Mar 2, 2016

The branch has been to fix the conflict. The difference is now much smaller, as it only makes the cfg built for --print cfg match the one used for compilation.

As pointed out by @alexcrichton, the flags that are added to the --print cfg output are only available through the use of a #![feature(...)] directive, but this seems to already affect other elements of the configuration (for example target_vendor).
Moreover --print cfg is only available in nightly, so this is not an issue right now.

@alexcrichton
Copy link
Member

Currently, however, --print cfg is riding the train to stable Rust. Could you perhaps modify this to only add the target features on nightly for now?

Gated cfg attributes are not available on the stable and beta release
channels, therefore they should not be presented to users of those
channels in order to avoid confusion.
@ranma42
Copy link
Contributor Author

ranma42 commented Mar 3, 2016

I managed to find how to filter out the gated cfg, so that --print cfg will also omit target_vendor. Moreover when either is stabilised the code will not require any special handling :)

@alexcrichton
Copy link
Member

@bors: r+ 4e46eee

Thanks!

bors added a commit that referenced this pull request Mar 3, 2016
Show `cfg` as possible argument to `--print` and make it so that `--print cfg` also outputs the `target_feature`s.

Should I also extend `src/test/run-make/print-cfg/Makefile` to check that `target_feature`s are actually printed?
@bors
Copy link
Contributor

bors commented Mar 3, 2016

⌛ Testing commit 4e46eee with merge e91f889...

@bors bors merged commit 4e46eee into rust-lang:master Mar 4, 2016
@ranma42 ranma42 deleted the printcfg branch April 20, 2016 07:59
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