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 -O and -g with -C before handling options #32399

Closed
wants to merge 1 commit into from

Conversation

sanxiyn
Copy link
Member

@sanxiyn sanxiyn commented Mar 21, 2016

Fix #32352.

@rust-highfive
Copy link
Collaborator

r? @Aatch

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

@sanxiyn
Copy link
Member Author

sanxiyn commented Mar 21, 2016

Travis failed because of conflict with #32369. (No textual conflict, but it changed Vec to slice.)

@alexcrichton
Copy link
Member

I'm not sure that this is the best way to implement a change like this, it's modifying pre-parsed options which could perhaps mess with parsing other options. For example what if an argument to another argument is the string -g or -O?

This is also a semantic change which may or may not be intended, I kinda forget the original motivation here...

cc @rust-lang/tools

@nrc
Copy link
Member

nrc commented Mar 21, 2016

I would like to see some discussion of #32352 before landing a PR. Perhaps an internals thread? Given that it is the interface to the compiler, maybe even an RFC.

@nrc
Copy link
Member

nrc commented Mar 21, 2016

Sorry, I think I misunderstood that issue, it's not actually proposing anything new, just better handling overlap. In which case it seems more like just a bug.

@sanxiyn
Copy link
Member Author

sanxiyn commented Mar 29, 2016

Rebased.

@alexcrichton
Copy link
Member

This continues to not handle the case I mentioned which I would personally consider a blocker for this.

@sanxiyn
Copy link
Member Author

sanxiyn commented Mar 29, 2016

For example what if an argument to another argument is the string -g or -O?

Is this possible? I failed to come up with such arguments: Vec<String>.

@alexcrichton
Copy link
Member

This may involve tweaking getopts perhaps to learn about the position of arguments. Either that or we could just change the semantics so the opt level is the max, not the rightmost one (which makes more sense to me at least)

@sanxiyn
Copy link
Member Author

sanxiyn commented Mar 30, 2016

"max not rightmost" would be different from GCC.

$ gcc -S test.c -o - -g2 -g0 | grep '\.debug_info' # no output
$ gcc -S test.c -o - -g0 -g2 | grep '\.debug_info'
.section .debug_info

@alexcrichton
Copy link
Member

Right, it's just easier to implement that a modification to getopts to track where each option came from positionally which I believe is the other alternative here.

@MagaTailor
Copy link

What about this case? (message is printed in plain text, no hint it's an error)

rustc: for the -force-target-max-vector-interleave option: may only occur zero or one times!

after providing -C llvm-args=-force-target-max-vector-interleave= twice on the command line.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with comments addressed!

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.

6 participants