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

Libs: Unify concat and concat_vec #18561

Closed
wants to merge 1 commit into from

Conversation

aturon
Copy link
Member

@aturon aturon commented Nov 3, 2014

We've long had traits StrVector and VectorVector providing
concat/connect and concat_vec/connect_vec respectively. The
reason for the distinction is that coherence rules did not used to be
robust enough to allow impls on e.g. Vec<String> versus Vec<&[T]>.

This PR consolidates the traits into a single Concat trait provided by
slice and the preldue (where it replaces StrVector, which is
removed.)

The VectorVector trait is retained in deprecated form.

[breaking-change]

We've long had traits `StrVector` and `VectorVector` providing
`concat`/`connect` and `concat_vec`/`connect_vec` respectively. The
reason for the distinction is that coherence rules did not used to be
robust enough to allow impls on e.g. `Vec<String>` versus `Vec<&[T]>`.

This PR consolidates the traits into a single `Concat` trait provided by
`slice` and the preldue (where it replaces `StrVector`, which is
removed.)

The `VectorVector` trait is retained in deprecated form.

[breaking-change]
@aturon
Copy link
Member Author

aturon commented Nov 3, 2014

The name Concat for this trait is somewhat arbitrary; suggestions welcome.

@huonw
Copy link
Member

huonw commented Nov 3, 2014

FWIW, I've been thinking that these might be better as freestanding functions that then take iterator, e.g.

impl String {
    fn concat<S, It>(iter: It) -> String
        where S: Str, It: Iterator<S> { ... }
}

This avoids one having to allocate a temporary vector. I guess it may cost the performance gained by preallocation.

@aturon
Copy link
Member Author

aturon commented Nov 3, 2014

@huonw We could probably even set up yet another overload of FromIterator, a trick I rather like -- but I worry about discoverability with that.

The approach you're suggesting is very slightly less ergonomic (though it would be better with IntoIterator), but not enough to cause major worries.

I'm basically happy to go in any of these directions; I mostly just want concat_vec to die.

pinging @alexcrichton for thoughts.

@aturon
Copy link
Member Author

aturon commented Nov 3, 2014

Update: @huonw and I chatted a bit on IRC, and feel that the FromIterator direction is a good one to go in ultimately -- especially as part of the general trend toward iterators. In particular, we can advertise that one should be familiar with the FromIterator implementations, since there are so many goodies there.

That said, I think we can/should land this PR as-is as an incremental improvement, and do some performance testing with the iterator approach. That will also involve adding an additional adapter method like the previously-proposed intersperse in order to implement connect.

@alexcrichton
Copy link
Member

I'm a huge fan of pushing as much as possible into FromIterator, I've loved the ability to collect an iterator of Result<T, Err> into Result<Vec<T>, Err> many times before! Could you elaborate a little more on how you envision that for concatenation though? I'm not sure I see how it'd be easily applicable to this.

I do agree with @huonw though that when concatenating items I find myself more often than not collecting an iterator into a Vec and then calling concat. Not only is it less efficient, but it's also less ergonomic b/c I have to write .collect::<Vec<_>>() (and often the _ isn't enough).

This is also borderline "one extra superfluous trait" in the stdlib as it seems to largely just be adding methods to slices?

I suppose I'd be more in favor of what @huonw suggested with Vec::concat and String::connect as I personally would find it more ergonomic as I'm usually connecting iterators anyway.

@alexcrichton
Copy link
Member

ping @aturon

@aturon aturon closed this Dec 4, 2014
@aturon
Copy link
Member Author

aturon commented Dec 4, 2014

Closing in favor of an iterator-based proposal.

lnicola pushed a commit to lnicola/rust that referenced this pull request Dec 11, 2024
…variable

Add macro expansion test for raw variable names
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.

3 participants