Use &[T] instead of &Vec<T> where possible

[petertseng]

I believe this covers the following Clippy lints:

1. writing &Vec<_> instead of &[_] involves one more reference and cannot be used with non-Vec-based slices.

• Where this was encountered, the signatures were changed from Vec<T> to &[T] in one commit; the resulting code compiles. Then a second commit was additionally made to change uses of vec! into slices, which is not necessitated by the first commit by the commit but is enabled by it.

2. this argument is passed by value, but not consumed in the function body

• Where this was encountered, the signatures were changed from Vec<T> to &[T] and the usages were changed from vec! to slices in one commit, since both changes need to happen together for the tests to compile.

3. useless use of vec!

• Where this was encountered, uses of vec! were changed to slices.

[petertseng]

I wonder if it can be done without the collect. First attempt, note that simply remove it completely will result in:

error[E0308]: mismatched types
  --> src/lib.rs:45:24
   |
45 |           CustomSet::new(&self.collection
   |  ________________________^
46 | |                             .iter()
47 | |                             .cloned()
48 | |                             .filter(|c| other.contains(c))
   | |__________________________________________________________^ expected slice, found struct `std::iter::Filter`
   |
   = note: expected type `&[T]`
              found type `&std::iter::Filter<std::iter::Cloned<std::slice::Iter<'_, T>>, [closure@src/lib.rs:48:37: 48:58 other:_]>`

[shingtaklam1324]

Yes. Vec::retain exists where it only keeps the items which meet the predicate. It does operate on &mut Vec though so the Vec will need to be cloned.

fn intersection(&self, other: &Self) -> CustomSet<T> {
    let mut collection = self.collection.clone();
    collection.retain(other.contains)
    CustomSet::new(collection)
}

docs

As well as that, there seems to be no reason for |c| other.contains(c) as it is equivalent to other.contains as far as I can tell.

[petertseng]

I think https://stackoverflow.com/questions/37801375/filter-a-slice-into-a-vector may give some hints as how to answer my question

Vec::retain [...] does operate on &mut Vec

Then I think it will not serve the goal (not having to create a temporary variable)

I admit I did not state my goal very precisely, therefore this is my fault that it did not serve the goal.

As well as that, there seems to be no reason for |c| other.contains(c) as it is equivalent to other.contains as far as I can tell.

True story, but I'm not allowed to make that change in this PR since it's out of scope. Good change to put in another PR.

[petertseng]

I think what I want is https://stackoverflow.com/questions/34969902/how-to-write-a-rust-function-that-takes-an-iterator

fn do_things(foos: &[usize]) {
    println!("Thanks for giving me {:?}", foos);
}

fn do_more_things<'a, I>(foos: I) where I: IntoIterator<Item = &'a usize> {
    for foo in foos {
        println!("Thanks for giving me more {}", foo);
    }
}

fn main() {
    let foos = [5, 10];
    
    // Works. Great!
    do_things(&foos);
    // Nice, both of these work!
    do_more_things(&foos);
    do_more_things(foos.iter());
    
    let even_foos: Vec<_> = foos.iter().filter(|f| *f % 2 == 0).cloned().collect();
    // Works too. All right.
    do_things(&even_foos);
    do_more_things(&even_foos);

    // Shall we do it without collect() ?
    //let even_foos2 = foos.iter().filter(|f| *f % 2 == 0).cloned();
    // I don't think there's a way to make this one work.
    //do_things(&even_foos2);
    // This is fine though.
    do_more_things(foos.iter().filter(|f| *f % 2 == 0));
}

[petertseng]

Hmm, maybe I misunderstood. I am looking in #400 (comment)

idiomatic code would be to specify the inputs as Vec<Vec<u64>> and then pass the reference and let the Deref trait do the work for you.

In that case, I'm making a few changes.

[petertseng]

Actually, I think I'll stop to ask questions before making changes.

The question I have right now is:

Clippy for sure asks for any &vec![a, b, c] to be changed to &[a, b, c].

If we have something on two separate lines:

let xs = vec![a, b, c];
do_thing_with(&xs);

Should we take a similar tack of removing vec! ? For is this not the same as what Clippy is suggesting to us?

If we do, assuming we still keep it on separate lines (perhaps the line would be too long if moving everything to one line), which form shall we change it to?

Choice 1:

let xs = &[a, b, c];
do_thing_with(xs);

Choice 2:

let xs = [a, b, c];
do_thing_with(&xs);

[coriolinus]

I am looking in #400 (comment)

There was a key pair of follow-up comments in #400, which are now hidden by default: 1, 2. I'd given the submitter bad advice earlier in that thread.

Should we take a similar tack of removing vec! ? For is this not the same as what Clippy is suggesting to us?

If it is reasonable for the list to ever need to grow, it should be a Vec. Otherwise, using a slice literal would be fine. Note that the question of whether it is reasonable for the list to ever need to grow has very little to do with whether or not it does so within the confines of the exercise; it has more to do with the model which the exercise asks the student to construct.

If we implement slice literals, I prefer your choice 1. The reasoning is that slices are immutable. Under choice 1, students who want to attempt to mutate them will need to write more: let mut xs = &mut [1, 2, 3];, as opposed to let mut xs = [1, 2, 3];. The repetition there should help clue them in that they are doing something wrong.

[petertseng]

OK. In that case I've confirmed that the commits in this PR which make the relevant change from Vec to slices are justified (the list will never need to grow) and they use choice 1. I recorded the relevant reasoning in individual commit messages. Commit contents have not changed, only individual commit messages.

[petertseng]

I'm moving the custom-set change into its own PR. As demonstrated by the fact that I had to change the example, the custom-set change is a change to the student-facing API. I expect all other changes to not affect students, and I've chosen to keep changes that likely need less discussion separate from changes that will need more discussion.

[petertseng]

I will have to rebase #434 after merging this, but that's fine, I was prepared to do so!

I think these changes are uncontroversial so with the Approval as assurance that I didn't make an obvious mistake somewhere it's time to merge it.