Implement Default on SnapshotVec, UnificationTable and unification stores.
#11
Conversation
src/unify/backing_vec.rs
Outdated
| @@ -17,6 +17,8 @@ pub trait UnificationStore: ops::Index<usize, Output = VarValue<Key<Self>>> + Cl | |||
| type Value: UnifyValue; | |||
| type Snapshot; | |||
|
|
|||
| // FIXME(eddyb) (not backwards-compat) this should | |||
| // use a `Default` bound on the trait instead. | |||
There was a problem hiding this comment.
You might as well change this to use Default if you want. We can always issue a new major version.
| @@ -75,13 +75,20 @@ pub trait SnapshotVecDelegate { | |||
| fn reverse(values: &mut Vec<Self::Value>, action: Self::Undo); | |||
| } | |||
|
|
|||
| impl<D: SnapshotVecDelegate> SnapshotVec<D> { | |||
| pub fn new() -> SnapshotVec<D> { | |||
| // HACK(eddyb) manual impl avoids `Default` bound on `D`. | |||
There was a problem hiding this comment.
Nit: I don't like the capital HACK -- using a manual impl isn't a hack -- using derive would be, given the way that Rust works right now, since it'd be easy but wrong.
I'd rather say "Use a manual impl to avoid unnecessary Default bound on D."
There was a problem hiding this comment.
What's wrong is the way Rust works right now, IMO.
There was a problem hiding this comment.
I agree :) it's just that leaving a comment here won't change it!
There was a problem hiding this comment.
wasn't there an RFC about this, giving some more convenient way to opt out, at least?
There was a problem hiding this comment.
I prefer to tag the places that could/should be fixed later.
src/unify/mod.rs
Outdated
| @@ -236,6 +236,8 @@ impl<K: UnifyKey> VarValue<K> { | |||
| // Option<U>:LatticeValue. | |||
|
|
|||
| impl<S: UnificationStore> UnificationTable<S> { | |||
| // FIXME(eddyb) (not backwards-compat) this should | |||
| // use a `Default` bound on `S` instead. | |||
There was a problem hiding this comment.
I'd rather just have UnificationStore require Default. It's not a trait that others outside the crate are meant to implement.
|
@eddyb yeah, once we merge, I can push a new release to crates.io |
eddyb
force-pushed
the
none-of-you-are-free-of-default
branch
from
e2c335f to
8dc607d
Compare
|
Oh @nikomatsakis didn't make |
This is needed for rust-lang/rust#52591. How will we update the dependency, do we need a release?