custom-set: Use &[...] instead of vec![...]

[petertseng]

The current tests of CustomSet pass in an owned Vec to CustomSet::new.

This actually seems to make sense, as it allows a FromIterator approach
and avoids cloning.

Note that this doesn't obviate the need for the T to be Clone, because
unions will surely require cloning.

The alternative approach expressed by this PR is to state that new
will not consume the input, instead cloning its elements.

The example had to be reworked to make this work, so this affects the
interface students are expected to adhere to.

I (the author of this commit) am no longer sure this is a good idea, so
let's have some discussion on it.

[petertseng]

Notes:

Current CustomSet::new interface was established in #114. The original #19 only had new that took no arguments (thus creating an empty set).

[coriolinus]

Given that passing in an owned data structure is reasonable for the constructor for custom set. Given that it is better to pass a slice than a Vec to enable other sliceable types. Might it work to pass an owned slice to the constructor?

I honestly don't know if rustc allows this. Sorry I haven't simply tested this myself; time remains precious. If it compiles, I think that might be the way to go.

[coriolinus]

Thinking about it, I'm fairly certain that there are certain intrinsic difficulties with the concept of an owned slice, and rustc will not allow it.

One might imagine an implementation in which CustomSet simply contained references to the data, but that poses its own problems. In general, a set should expect to own its contained data.

I think that this approach, passing in a slice and cloning as required, is as close to ideal as we can reasonably expect. It does require cloning on set creation, but for Copy types such as are actually used in the tests, there's no intrinsic performance penalty.

[petertseng]

I'll take this Approval to mean that the proposed state is better than the current state.

I've been reflecting more and if I decide I agree (I mentioned I wasn't sure), I will indeed merge.

I will also discuss whether this is the desired final state, OR if there is another change that should be made after this.

I'm trying to answer this by answering the question "Does this match the API of the standard Rust containers?"

At least for Vec, it does not. Vec only provides a zero-argument new. If we wish to construct a Vec that has the elements of some other iterable, I typically see https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.collect being used (taking advantage of https://doc.rust-lang.org/std/vec/struct.Vec.html#impl-FromIterator%3CT%3E).

I now see that pursuing that strategy would lead back into the original as shown in #19 ! I believe the change was made in #114 because it was not clear that FromIterator needed to be implemented to make collect work. We can resolve that with a stub. I think at some point it will be good to build a collection that follows this practice. But it doesn't have to be custom-set. I'll think about whether there are any exercises that can be used to demonstrate it.

[petertseng]

I have decided this is better than the original, and the reason that convinces me will be:

Given that it is better to pass a slice than a Vec to enable other sliceable types

Emphasis mine.

I acknowledge that this API is still significantly different from https://doc.rust-lang.org/std/collections/struct.HashSet.html . We can revisit whether it's better to return to it later.

[petertseng]

(the stub was empty, so I did not need to rebase to perform a stub compilation check)

[petertseng]

Given that it is better to pass a slice than a Vec to enable other sliceable types

I think this should be stated differently. We don't provide a stub (yet! but we could!). If we did, then we would probably make the stub suggest &[T] in which case that reasoning applies.

In the absence of any suggestion that a stub makes, students were probably able to write some implementation that uses IntoIterator:

#489 works before this PR. The equivalent after this PR is #456.

[petertseng]

I decided not to spend a particularly long time thinking about this. In my view, the important part of this exercise is implementing the interesting operations on the set, not how the set itself is constructed. Thus, we should make set construction relatively convenient, which implies we should not use the original API of #19, and that it does not particularly matter whether this PR was applied.

If wanting to demonstrate converting various iterable types can/should convert to/from one another, it would be interesting to have another exercise that shows that.