run: Process::new() should take &[Str] instead of &[~str] for args #8203
Conversation
|
@graydon: Upon thinking more about the ugly code in Alternatively I could reimplement Thoughts? |
|
@graydon: I deleted your r+ comment due to the above issue. |
|
@graydon: r? I fixed |
|
Needs a rebase. Otherwise r=me, yeah. This is something I thought wouldn't be a problem at first since you were asking about a code change when the incoming values were all |
|
r? @graydon |
The method .into_owned() is meant to be used as an optimization when you need to get a ~str from a Str, but don't want to unnecessarily copy it if it's already a ~str. This is meant to ease functions that look like fn foo<S: Str>(strs: &[S]) Previously they could work with the strings as slices using .as_slice(), but producing ~str required copying the string, even if the vector turned out be a &[~str] already. I don't have any concrete uses for this yet, since the one conversion I've done to `&[S]` so far (see PR #8203) didn't actually need owned strings. But having this here may make using `Str` more attractive. It also may be worth adding an `into_managed()` function, but that one is less obviously useful than `into_owned()`.
|
The bad news is that we just (today) agreed to take the patch that removes the trailing nulls from slices and sets their length back to always cover only valid chars. This means that (a) you'll always need to copy and (b) if you use that probe-one-past-the-end code you copied and pasted in here, it'll start causing memory faults. Sorry about this mess. Confusion around the trailing nulls is exactly the thing we are trying to get rid of, by removing the feature. |
|
Holding this patch until #8296 lands, at which point I will rewrite this on top of it. |
|
@graydon There was one error in librust, which I didn't catch because I only ran the libstd tests instead of doing a full build. My bad. r? |
|
Rebased yet again. |
|
@kballard Needs rebase (and the test failure may be legitimate, I'm not sure) |
|
Rebased. Don't understand the test failures though. Nothing I did should affect Windows linkage, or should add calls to Windows functions that weren't previously used. |
|
I have no idea what's causing the Windows failures: |
|
The referenced calls are all used in |
|
@kballard Needs a rebase in any case. |
|
@catamorphism I can rebase it yet again, but that won't solve the windows problem. My best guess right now is that there's something going on here where the fact that it's generic means that libstd isn't actually linking against the FFI functions (because it contains the AST for the function instead of the actual compiled executable code), and then when libextra tries to call it, it hits the undefined symbols. |
|
It appears that process spawning has changed significantly. I'm going to re-write this PR on top of the new approach, but I don't know if the Windows issue will continue to happen. I wish I had some way of testing without going through the full r+ process. |
|
Looks like the new approach no longer defines the Windows FFI functions as part of the process stuff, instead relying on libuv to deal with it, so I hope that means it won't happen again. |
|
I have a rewritten version now that passes the stage2 libstd tests, but I'm running |
|
Don't review yet. Pushing here because I'm having a compilation issue. |
|
Ok this new commit passes |
|
r? |
|
Sorry about |
|
@alexcrichton Why was it reverted? Was that the source of the crashes on the builders? Anyway, going back to the previous implementation of this is just going to bring back the Windows test failures. My guess right now is there's a problem with declaring extern fns inside a generic fn. I could try refactoring those declarations to be outside the generic fn, but I have no way of test Windows short of going through the whole r+ -> full make check process again. |
|
It was both the source of a massive regression on windows and I also suspect that it was causing all the segfaults. For you extern fn inside a generic fn problem, I'd be interested in seeing if #8843 fixes that issue. |
|
Rebased. This presumably is going to require #8843 to work on Windows though. |
|
@huonw Great. This can be reviewed again now. |
|
Damn. #8843 didn't fix it. |
|
Closing due to lack of activity. Please reopen a new pull request if you get the chance to work on this more -- thanks! |
|
It seems #9055 may be the cause of the Windows errors. |
New lint: `iter_overeager_cloned` Closes rust-lang#8202 changelog: New lint: [`iter_overeager_cloned`]
Fixes #7928.
As a side-effect, this no longer clones all the arg strings on unix.