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.

Sign up for GitHub

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

Jump to bottom

Implement --remap-path-prefix #48359

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Implement --remap-path-prefix #48359

merged 1 commit into from
Mar 1, 2018

Conversation

jsgf
Copy link
Contributor

@jsgf jsgf commented Feb 19, 2018

Remove experimental -Zremap-path-prefix-from/to, and replace it with
the stabilized --remap-path-prefix=from=to variant.

Implementation for #41555

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2018
@jsgf jsgf force-pushed the remap-path-prefix branch from 409cd00 to 563ed18 Compare February 19, 2018 21:04
michaelwoerister
michaelwoerister reviewed Feb 20, 2018
View reviewed changes
src/librustc/session/config.rs Outdated
.into_iter()
.map(|remap| {
let mut parts = remap.rsplitn(2, '=');
match (parts.next(), parts.next()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The evaluation of components in tuple constructors is probably defined, but I still find this hard to read. And it's probably the wrong order because the iterator starts at the end :) Could you rewrite this to something like:

let mut parts = ...;
let to = parts.next();
let from = parts.next();
match (from, to) { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything that implements FromIterator<Item=T> for Option<(T,...)>? I keep ending up in situations where it would be useful (mostly parsing with split), but I suspect it would clash with an existing FromIterator for Option<T> (unless specialization helps?).

@michaelwoerister
Copy link
Member

Thanks a lot, @jsgf! The implementation looks good to me, with the bug fixed.

I think we also need to update some documentation, right @rust-lang/docs? There's an entry for the feature in the unstable book. Where should this be moved to?

@jsgf
Copy link
Contributor Author

jsgf commented Feb 20, 2018

Yes, documentation was my next question.

@jsgf jsgf closed this Feb 20, 2018
@jsgf jsgf force-pushed the remap-path-prefix branch from 563ed18 to d0f8e29 Compare February 20, 2018 17:43
@jsgf jsgf reopened this Feb 20, 2018
@jsgf jsgf force-pushed the remap-path-prefix branch from 65e2200 to ed3e10d Compare February 20, 2018 21:08
@jsgf
Copy link
Contributor Author

jsgf commented Feb 20, 2018 •
edited
Loading

Removed unstable book documentation and updated rustc.1 man page, which seems to be the only place I could find written documentation on command-line options.

@jsgf jsgf force-pushed the remap-path-prefix branch from ed3e10d to b2f4104 Compare February 20, 2018 21:17
@jsgf jsgf changed the title rust: implement --remap-path-prefix Implement --remap-path-prefix Feb 20, 2018
@jsgf jsgf force-pushed the remap-path-prefix branch from b2f4104 to ffd8ea7 Compare February 20, 2018 23:16
@steveklabnik
Copy link
Member

Yes, for now that's true. We haven't created a space for rustc docs yet; we wanted to.

@michaelwoerister
Copy link
Member

Alright then: @bors r+

Thanks for pushing this over the finish line, @jsgf!

@rust-lang/core, @rust-lang/compiler, @rust-lang/dev-tools: We are stabilizing path prefix remapping!

@bors
Copy link
Contributor

bors commented Feb 21, 2018

📌 Commit ffd8ea7 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2018
@michaelwoerister michaelwoerister added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 21, 2018
@jsgf
Copy link
Contributor Author

jsgf commented Feb 22, 2018

Ah, I just found a problem - it looks like having this option on the command line changes the crate hash.

@jsgf
Copy link
Contributor Author

jsgf commented Feb 22, 2018

Looks like #48019/#48162 is still an issue.

@jsgf
Copy link
Contributor Author

jsgf commented Feb 22, 2018

Rebasing to master seems to have fixed it, so maybe I just didn't have the other change before.

@jsgf
Implement --remap-path-prefix
56a6828 
Remove experimental -Zremap-path-prefix-from/to, and replace it with
the stabilized --remap-path-prefix=from=to variant.

This is an implementation for issue of rust-lang#41555.
@jsgf jsgf force-pushed the remap-path-prefix branch from ffd8ea7 to 56a6828 Compare February 22, 2018 23:13
@sanxiyn
Copy link
Member

sanxiyn commented Feb 26, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Feb 26, 2018

📌 Commit 56a6828 has been approved by sanxiyn

@Manishearth Manishearth mentioned this pull request Feb 28, 2018
Merged
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 28, 2018
@Manishearth
Rollup merge of rust-lang#48359 - jsgf:remap-path-prefix, r=sanxiyn
fac7d7c 
Fixes rust-lang#47311.
r? @nrc
bors added a commit that referenced this pull request Feb 28, 2018
@bors
Auto merge of #48615 - Manishearth:rollup, r=Manishearth
0e3c9bb 
Rollup of 10 pull requests

- Successful merges: #48355, #48359, #48380, #48419, #48420, #48461, #48522, #48570, #48572, #48603
- Failed merges:
@bors bors merged commit 56a6828 into rust-lang:master Mar 1, 2018
@jsgf jsgf deleted the remap-path-prefix branch March 1, 2018 06:55
@infinity0 infinity0 mentioned this pull request May 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwoerister michaelwoerister michaelwoerister left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jsgf @rust-highfive @michaelwoerister @steveklabnik @bors @sanxiyn @petrochenkov