hashmap: Remove .consume() methods

[bluss]

Updated all users of HashMap, HashSet ::consume() to use
.consume_iter().

Since .consume_iter() takes the map or set by value, it needs awkward
extra code to in librusti's use of @mut HashMap, where the map value can
not be directly moved out.

Addresses issue #7719

[bluss]

See issue #7719 for the caveat, consume_iter() is awkward with @mut HashMap, since you can't move out of it.

[alexcrichton]

Why not have the same semantics as today where it simply empties the hashmap, but the hashmap is still usable after consumption?

[bluss]

We could, but it would not be consistent with vec's consume_iter. Also both alternatives have uses. By-value consume iter is useful if you want to pass the iterator on as an owned value.

[alexcrichton]

This is always going to be a by-value iterator. There really aren't that many consume iterators and the one I recently added to SmallIntMap takes &mut self instead of self. Vectors are a case where I think it makes more sense to take self instead of &mut self because you're actually moving out of the contents of the memory the vector owns. For a hashmap, it just owns the vector, and the vector then owns the actual data.

Plus, sometimes it's kind of nice to keep the same hashmap around for multiple iterations of some code. Between iterations you consume all key/value pairs, but you don't have to re-allocate the entire map (and re-seed it from the task-local RNG).

I'd still vote for taking &mut self instead of self, but others should probably weigh in other than me.

As a side note, I named the method consume on SmallIntMap because as soon as advance goes away I want it to look like:

for map.consume |(key, value)| {
  // ...
}

Having the _iter prefix seems not necessary when there's no other consume method.

[bluss]

I think all consume iterators should be consistent. They are not only for direct use but also for use with the IteratorUtil methods.

[alexcrichton]

What does it mean to be consistent and to be used with IteratorUtil? Anything which implements Iterator can be used with IteratatorUtil.

Consistency among implementations is an entirely different matter. So far maps are on the trend to take &mut self and vectors take self, but there's not really data in either direction. If this does end up taking self it's probably a good idea to change SmallIntMap to be consistent as well.

[huonw]

@alexcrichton, fwiw, there's a pile of _iter suffixes that can disappear once all the non-iterators disappear, so that change can be done across the entire codebase later.

[alexcrichton]

@huonw, I agree, although if this is removing the consume method as well (which it is), then there's no reason to add a temporary _iter suffix

[bluss]

the IteratorUtil thing is just that, for direct use with for, by-value or by-mut-ref doesn't matter, but when you encapsulate the hashmap inside an IteratorUtil combinator, it does make a difference for how that value can be passed around.

Since the awkward dance with util::replace only appears where the code is using @mut HashMap, I think this is fine as it is.

[alexcrichton]

@blake2-ppc, I think there's a disconnect between what you and I are thinking, so here's what I thought the consume method could be like

fn consume(&mut self) -> SomeIterator<(K, V)> {
  let arr = util::replace(&mut self.slots, ~[]);
  arr.consume_iter().filter_map(|x| match x { Some(slot) => Some((slot.key, slot.value)), None => None })
}

There's no constraints on what you're iterating over or anything like that. The only difference is that you can still use the hashmap after you consume it. The values owned by the hashmap are still moved into the iterator that you return.

[bluss]

Nice, didn't look at your Smallintmap. That behavior is the best of both worlds, only problem is the extra allocation and copy of ~[], can the compiler optimize that away (when applicable) if the method is inline?

[alexcrichton]

Hmm... That is a good point. I doubt that the compiler would optimize away the allocation because that's a very complicated function to determine is useless.

That actually puts me more in favor of what it is today actually because "consumption" essentially means transferring ownership and it shouldn't really involve reallocations.

[alexcrichton]

If you rename consume_iter to consume I'd be willing to r+ this (others could r+ now though!)

[bluss]

Renamed to .consume(), no other changes.

[Kimundi]

@alexcrichton would you mind if consume() gets renamed to move_iter() ?