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

std::run::Process::new() should take &[&str] instead of &[~str] #7928

Closed
metajack opened this issue Jul 20, 2013 · 9 comments · Fixed by #13954
Closed

std::run::Process::new() should take &[&str] instead of &[~str] #7928

metajack opened this issue Jul 20, 2013 · 9 comments · Fixed by #13954
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@metajack
Copy link
Contributor

Right now you have to clone everything into an array instead of just passing pointers. It would be nice if it did the cloning behind the scenes if it really must be done, or better yet, avoid it altogether.

@metajack
Copy link
Contributor Author

nominating production ready

@huonw
Copy link
Member

huonw commented Jul 20, 2013

This could probably be <S: Str>(..., &[S], ...), so that it can be called with &[~str] and &[&str] (etc) without having to create a new vector.

@catamorphism
Copy link
Contributor

Just a bug, de-nominating

@emberian
Copy link
Member

I just checked how Process::new works on Linux, and on that platform we cannot do zero-copy exec with &[&str], because the strings need to be null terminated. And we need to do copies anyway for Windows. Since there is going to be an allocation involved already, I do not think this should happen. In general we try to avoid hiding allocations in our APIs.

@huonw
Copy link
Member

huonw commented Jan 14, 2014

Do the internals allocate with the current design, or do they reuse the ~str where possible?

(In any case, I guess these really should take CStrings, or something generic.)

@lilyball
Copy link
Contributor

@cmr: We shouldn't be forcing callers to allocate just because on one platform we need to allocate internally.

@huonw
Copy link
Member

huonw commented Jan 14, 2014

@kballard we need null-termination on Unix platforms.

@lilyball
Copy link
Contributor

@huonw: Yes, but that's no excuse for requiring the caller to provide a ~str. Especially because we may still need to reallocate even with a ~str, if there's no extra capacity in the current allocation.

@emberian
Copy link
Member

It's not one platform. It's also OS X and Windows. If it isn't true on
every platform, I'll be surprised. (libuv takes them as a char** too, and
does use CString internally)

But, on Windows, we build a complete command-line string entirely, it
doesn't just want an array of strings. And it doesn't want null termination.

On Mon, Jan 13, 2014 at 7:42 PM, Kevin Ballard notifications@git.luolix.topwrote:

@cmr https://github.com/cmr: We shouldn't be forcing callers to
allocate just because on one platform we need to allocate internally.


Reply to this email directly or view it on GitHubhttps://github.com//issues/7928#issuecomment-32228526
.

bors added a commit that referenced this issue May 15, 2014
## Process API

The existing APIs for spawning processes took strings for the command
and arguments, but the underlying system may not impose utf8 encoding,
so this is overly limiting.

The assumption we actually want to make is just that the command and
arguments are viewable as [u8] slices with no interior NULLs, i.e., as
CStrings. The ToCStr trait is a handy bound for types that meet this
requirement (such as &str and Path).

However, since the commands and arguments are often a mixture of
strings and paths, it would be inconvenient to take a slice with a
single T: ToCStr bound. So this patch revamps the process creation API
to instead use a builder-style interface, called `Command`, allowing
arguments to be added one at a time with differing ToCStr
implementations for each.

The initial cut of the builder API has some drawbacks that can be
addressed once issue #13851 (libstd as a facade) is closed. These are
detailed as FIXMEs.

## Dynamic library API

`std::unstable::dynamic_library::open_external` currently takes a
`Path`, but because `Paths` produce normalized strings, this can
change the semantics of lookups in a given environment. This patch
generalizes the function to take a `ToCStr`-bounded type, which
includes both `Path`s and `str`s.

## ToCStr API

Adds ToCStr impl for &Path and ~str. This is a stopgap until DST (#12938) lands.

Until DST lands, we cannot decompose &str into & and str, so we cannot
usefully take ToCStr arguments by reference (without forcing an
additional & around &str). So we are instead temporarily adding an
instance for &Path and ~str, so that we can take ToCStr as owned. When
DST lands, the &Path instance should be removed, the string instances
should be revisted, and arguments bound by ToCStr should be passed by
reference.

FIXMEs have been added accordingly. 

## Tickets closed

Closes #11650.
Closes #7928.
flip1995 pushed a commit to flip1995/rust that referenced this issue Nov 23, 2021
…, r=flip1995

Reference `clippy_utils` docs on nightly-rustc and some other documentation updates

The `clippy_utils` crate is now part of the nightly-rustc documentation. See [**very beautiful documentation**](https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/). This PR references them in our documentation and updates some other documentation.

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
5 participants