rust-lang / datafrog

A lightweight Datalog engine in Rust
Apache License 2.0
796 stars 43 forks source link

idiomatic collect() in map.rs #12

Closed sscdotopen closed 6 years ago

frankmcsherry commented 6 years ago

This seems smart to me! Would probably allow for correct sizing of the allocation too, whereas the old version probably doesn't? Anyhow.

Might it be even more idiomatic to write:

let results: Vec<T2> = input.recent
         .borrow()
         .iter()
         .map(logic)
         .collect();

which is just me being .. not very helpful. But, looks great to me either way. Not clear what the approval story is for this repo, but I'll happily ok it if no one else has any comments. :)

sscdotopen commented 6 years ago

Hi Frank,

You are right, collect() will call from_iter for Vec which checks the size_hint of the iterator to see if it can preallocate enough slots before pushing the elements. The old implementation did not use this optimization.

frankmcsherry commented 6 years ago

Cool! Actually, I have a "idiomaticy" question that maybe @lqd can chime in on: using impl Fn here is slick, but when there are other generic parameters it means they cannot be specified (attempts to specify T1 or T2 will fail, because you can't specify or elide the impl trait bit). Is there a Rust position on e.g. "Don't use impl trait when there are other generic parameters"?

lqd commented 6 years ago

A good question indeed, and one that I will need to ask :) I don't remember such a position existing yet, but the impl Trait interaction, or lack thereof, with "turbofish" has come up before, and I also wonder the same thing. I'm away from computers for another week unfortunately :/

Just a nit: Vec<_> without the T2 could look nice as well :)

sscdotopen commented 6 years ago

What's the benefit of Vec<_> over Vec<T2> ? Doesn't it make the code harder to read?

lqd commented 6 years ago

To me, it's more of a signal that one's focusing on the type of the container, rather than the thing it contains and letting the compiler infer the rest. For this very simple case both work equally, but the thought would apply with more complicated types contained, where the second way would have clear benefits. I believe that's why you see this idiom regularly.

lqd commented 6 years ago

@frankmcsherry BTW I've asked on Discord here and apart from the possible breaking change issue when introducing an impl Trait argument, API design considerations came up (whether it is likely that the other generic parameters are "turbofished") but there doesn't seem to be such a position on impl Trait + other generic parameters today :)