Skip to content

Implement Default on SnapshotVec, UnificationTable and unification stores. #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ description = "Union-find, congruence closure, and other unification code. Based
license = "MIT/Apache-2.0"
homepage = "https://github.com/nikomatsakis/ena"
repository = "https://github.com/nikomatsakis/ena"
version = "0.9.3"
version = "0.10.0"
authors = ["Niko Matsakis <[email protected]>"]
readme = "README.md"
keywords = ["unification", "union-find"]
Expand Down
11 changes: 9 additions & 2 deletions src/snapshot_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong is the way Rust works right now, IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree :) it's just that leaving a comment here won't change it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't there an RFC about this, giving some more convenient way to opt out, at least?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to tag the places that could/should be fixed later.

impl<D: SnapshotVecDelegate> Default for SnapshotVec<D> {
fn default() -> Self {
SnapshotVec {
values: Vec::new(),
undo_log: Vec::new(),
}
}
}

impl<D: SnapshotVecDelegate> SnapshotVec<D> {
pub fn new() -> Self {
Self::default()
}

pub fn with_capacity(c: usize) -> SnapshotVec<D> {
SnapshotVec {
Expand Down
31 changes: 18 additions & 13 deletions src/unify/backing_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ type Key<S> = <S as UnificationStore>::Key;
/// Largely internal trait implemented by the unification table
/// backing store types. The most common such type is `InPlace`,
/// which indicates a standard, mutable unification table.
pub trait UnificationStore: ops::Index<usize, Output = VarValue<Key<Self>>> + Clone {
pub trait UnificationStore:
ops::Index<usize, Output = VarValue<Key<Self>>> + Clone + Default
{
type Key: UnifyKey<Value = Self::Value>;
type Value: UnifyValue;
type Snapshot;

fn new() -> Self;

fn start_snapshot(&mut self) -> Self::Snapshot;

fn rollback_to(&mut self, snapshot: Self::Snapshot);
Expand Down Expand Up @@ -51,16 +51,18 @@ pub struct InPlace<K: UnifyKey> {
values: sv::SnapshotVec<Delegate<K>>
}

// HACK(eddyb) manual impl avoids `Default` bound on `K`.
impl<K: UnifyKey> Default for InPlace<K> {
fn default() -> Self {
InPlace { values: sv::SnapshotVec::new() }
}
}

impl<K: UnifyKey> UnificationStore for InPlace<K> {
type Key = K;
type Value = K::Value;
type Snapshot = sv::Snapshot;

#[inline]
fn new() -> Self {
InPlace { values: sv::SnapshotVec::new() }
}

#[inline]
fn start_snapshot(&mut self) -> Self::Snapshot {
self.values.start_snapshot()
Expand Down Expand Up @@ -132,17 +134,20 @@ pub struct Persistent<K: UnifyKey> {
values: DVec<VarValue<K>>
}

// HACK(eddyb) manual impl avoids `Default` bound on `K`.
#[cfg(feature = "persistent")]
impl<K: UnifyKey> Default for Persistent<K> {
fn default() -> Self {
Persistent { values: DVec::new() }
}
}

#[cfg(feature = "persistent")]
impl<K: UnifyKey> UnificationStore for Persistent<K> {
type Key = K;
type Value = K::Value;
type Snapshot = Self;

#[inline]
fn new() -> Self {
Persistent { values: DVec::new() }
}

#[inline]
fn start_snapshot(&mut self) -> Self::Snapshot {
self.clone()
Expand Down
6 changes: 2 additions & 4 deletions src/unify/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ pub struct VarValue<K: UnifyKey> { // FIXME pub
/// cloning the table is an O(1) operation.
/// - This implies that ordinary operations are quite a bit slower though.
/// - Requires the `persistent` feature be selected in your Cargo.toml file.
#[derive(Clone, Debug)]
#[derive(Clone, Debug, Default)]
pub struct UnificationTable<S: UnificationStore> {
/// Indicates the current value of each key.
values: S,
Expand Down Expand Up @@ -237,9 +237,7 @@ impl<K: UnifyKey> VarValue<K> {

impl<S: UnificationStore> UnificationTable<S> {
pub fn new() -> Self {
UnificationTable {
values: S::new()
}
Self::default()
}

/// Starts a new snapshot. Each snapshot must be either
Expand Down