Make opts_present and opts_str in the getopts module take &[&str] instead of &[String]

[gsingh93]

There's no reason opts_present and opts_str need heap allocated strings.

[gsingh93]

Alright, I'll fix that.

[alexcrichton]

I'm not totally comfortable with exploding our interfaces everywhere with <T: Str> just yet. The trait Str was created as an implementation detail for the as_slice() function I thought, not to be a public-facing widely-used API.

Requiring &[&str] doesn't necessarily cut down on allocations because you have to allocate an array of borrowed slices somewhere (which is currently not done very often).

[lilyball]

I agree with @cmr, using <T: Str> is preferable for things like this. If the function internally ends up creating a Vec<&str> then it's pointless, but this code doesn't seem to do that, so taking <T: Str> does indeed cut down on allocations.

[gsingh93]

So actually I think using &str as the type makes sense, especially if auto slicing is on the roadmap (making passing Strings in easier). And for now, you can call .as_slice() for each element if you're using a Strings, which for this function seems less likely.

[emberian]

No, the T does not get moved, it's a slice (non-owning view) into an array
of T. You can't move out of a slice. &[String] cannot be converted
into &[&str] without using an equivalent to strs.iter().map(|x| x.as_slice().collect() -- that is, allocating a whole new vector.

On Tue, May 27, 2014 at 8:31 PM, Gulshan Singh notifications@github.comwrote:

So I actually don't see why <T: Str> is the right choice here. This is
how I think the interface you're proposing looks:

pub fn opts_present<T: Str>(&self, names: &[T]) -> bool

If that's correct, then don't the T in the array get moved, rendering
their original variables useless? I think using &str as the type makes
sense, especially if auto slicing is on the roadmap (making passing Strings
in easier).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14453#issuecomment-44362197
.

http://octayn.net/

[emberian]

It cannot work for arrays. You still need to allocate a new vector. String
and &str are different sizes and different representations.

On Tue, May 27, 2014 at 8:47 PM, Gulshan Singh notifications@github.comwrote:

@cmr https://github.com/cmr Yea, I figured that out and edited my
comment. But as I mentioned about auto slicing, if that is implemented
wouldn't you be able to pass a &[String] as a &[&str], or would it not
work for arrays? For now the user can just call .as_slice() on each of
the String elements before passing it. Currently, I have to call
to_strbuf() on each of my &str, which I think is worse.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14453#issuecomment-44362830
.

http://octayn.net/

[gsingh93]

@cmr I see now. I edited and deleted my previous comments before you replied, but it looks you replied to the email. I'm working on the change right now.

[emberian]

eugh github-via-email can be so annoying sometimes. sorry :(

[gsingh93]

Updated.

[emberian]

Needs a rebase

[emberian]

r=me though

[alexcrichton]

@cmr, Before r+'ing, what do you think of my earlier comment? Should we accept T: Str everywhere now? Should we document that it is a first-class trait that everyone should use ubiquitously?

[emberian]

I would hate to do that, because it's so, so ugly. In this case it's to
avoid unnecessary allocation. I'm certain that <T: Str> would serve only to
confuse beginners and bloat compile times. On the other hand, it would
sidestep the entire lack of String -> &str coercions in callers. I suppose
the eventual impl Str stuff could help with the ugliness.

On Fri, May 30, 2014 at 9:33 AM, Alex Crichton notifications@github.com
wrote:

@cmr https://github.com/cmr, Before r+'ing, what do you think of my
earlier comment? Should we accept T: Str everywhere now? Should we
document that it is a first-class trait that everyone should use
ubiquitously?

—
Reply to this email directly or view it on GitHub
#14453 (comment).

http://octayn.net/

[alexcrichton]

I do not believe that the paramount goal of Rust is to avoid all allocations wherever possible. This is an example I think of a location where avoiding an allocation barely matters because you're parsing options from the command line, which likely happens only once in a program.

This looks like it's starting to set the trend of "take T: Str wherever possible", which I am afraid of.

[gsingh93]

Rebased.

[emberian]

I agree with the hesitation but I'm certainly not against removing
allocation when the fix is relatively painless. I'll withhold my r+ for now
though.

On Fri, May 30, 2014 at 11:07 AM, Gulshan Singh notifications@github.com
wrote:

Rebased.

—
Reply to this email directly or view it on GitHub
#14453 (comment).

http://octayn.net/

[lilyball]

Implicit coercion of String -> &str doesn't help the case of &[&str], because you can't coerce a &[String] to &[&str].

This particular case is one that doesn't matter too much for allocation reasons, because it's largely just done once, but I actually think that using <T: Str> to take slices of strings is what we should be moving towards.

@alexcrichton A long time ago I had a PR to do something similar for Process::new() (and related methods), which ended up stalling out due to weird build issues on Windows. I ended up abandoning it because at that time I didn't have try access and it was too painful to try and figure out what was going on, but that PR was motivated by an issue explicitly requesting that behavior. Although in that case I believe it was taking whatever the predecessor to StrAllocating was (i.e. whatever had .into_owned() at that time).

[lilyball]

Found it. It actually was taking Str after all (I thought it used .into_owned() but it really just used .as_slice()). The issue in question was #7928, which actually doesn't ask for Str but @huonw's comment (back on Jul 20, 2013) suggested taking <S: Str>, which was not controversial at the time.

[alexcrichton]

Dealing with processes was a serious ergonomics issue because there were often many static strings that were used as arguments. That was an excellent impetus for the Command builder with a trait bound on the argument it takes.

Dealing with getopts where it gets its arguments 99% of the time from os::args() means that dealing with &str and String is much less of an ergonomics issue. It is also much rarer today to rely on implicit coercions now that Vec and String are ubiquitous and don't auto-borrow one way or the other.

The rationale for this PR, as stated in the description, is to save an allocation. This is a library which is never used in an inner loop, so I don't understand why saving an allocation should be a goal of this library.

As I stated before, the goal of rust is not to minimize allocations. If a library allocates, that does not mean that it is sub-par or needs improvement.

[emberian]

For implicit coercion I was referring to <T: Str> everywhere.

On Fri, May 30, 2014 at 11:38 AM, Kevin Ballard notifications@github.com
wrote:

Found it. It actually was taking Str after all (I thought it used
.into_owned() but it really just used .as_slice()). The issue in question
was #7928 #7928, which actually
doesn't ask for Str but @huonw https://github.com/huonw's comment (back
on Jul 20, 2013) suggested taking <S: Str>, which was not controversial
at the time.

—
Reply to this email directly or view it on GitHub
#14453 (comment).

http://octayn.net/

[lilyball]

@alexcrichton Ok, I agree, this particular library does not need to worry about allocations. My feeling is that encouraging a trend of <T: Str> to use &[T] is something that we should start doing in general, but this particular library is perhaps not the right place to establish that convention.

To that end, we should probably close this PR for now, but accept if it it's re-submitted after other libraries have established the convention.

[alexcrichton]

Ok, I'm going to close this for now, and we'll see how the conventions play out in the future.