Skip to content
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

Replace various lookups by one data structure #19

Open
danieldk opened this issue Oct 31, 2018 · 2 comments
Open

Replace various lookups by one data structure #19

danieldk opened this issue Oct 31, 2018 · 2 comments
Milestone

Comments

@danieldk
Copy link
Owner

There is a lot of overlap between the transition-specific lookup and feature table lookup. This can be factored out to one class that replaces Numberer.

Maybe this should be a separate crate, because it is generally useful.

@danieldk danieldk added this to the dpar 0.2 milestone Oct 31, 2018
@danieldk
Copy link
Owner Author

danieldk commented Nov 8, 2018

This turns out to be difficult to do elegantly generically. The problem is that in lookup methods, you want to take e.g. &str rather than &String (since it doesn't require the construction of a String). However, one then needs type parameters on lookup methods, such as:

fn lookup<Q: ?Sized>(&self, q: &Q) -> usize where T: Borrow<Q>

However, generic methods are not object safe. Since I use both boxed transition lookups and boxed feature lookups, this does not compile. And I am not sure whether I want to give up dynamic polymorphism for lookups and transitions.

An alternative that I am now thinking of is specifying both the owned and borrowed types as type parameters (note that we cannot use Borrow directly, since some times can be borrowed as multiple types) to avoid generic methods.

@sebpuetz

@danieldk
Copy link
Owner Author

danieldk commented Nov 8, 2018

I have found a more elegant solution, more later ;).

danieldk added a commit that referenced this issue Nov 9, 2018
This change introduces a single lookup table that replaces:

- TransitionLookup
- LookupTable
- MutableLookupTable

StoredLookupTable is simplified, but still exists as a wrapper around
FreezableLookup to simplify serialization.

FreezableLookup implements three traits:

- LookupValue: defines the methods to look up an identifier for a value
  and vice versa.
- LookupLen: defines the `len` method.
- Lookup: inherits LookupValue and LookupLen and is implemented for any
  type that implements both traits.

In my first iteration, I had a single lookup trait. However, this
sometimes led to type inference problems when the `len` method was used.
Since `len` does not use the borrowed type in its signature, the type
parameter for Lookup<B> could not be inferred. So, instead, we make the
`len` method part of a trait without type parameters.

Fixes #19.
danieldk added a commit that referenced this issue Nov 12, 2018
This change introduces a single lookup table that replaces:

- TransitionLookup
- LookupTable
- MutableLookupTable

StoredLookupTable is simplified, but still exists as a wrapper around
FreezableLookup to simplify serialization.

FreezableLookup implements three traits:

- LookupValue: defines the methods to look up an identifier for a value
  and vice versa.
- LookupLen: defines the `len` method.
- Lookup: inherits LookupValue and LookupLen and is implemented for any
  type that implements both traits.

In my first iteration, I had a single lookup trait. However, this
sometimes led to type inference problems when the `len` method was used.
Since `len` does not use the borrowed type in its signature, the type
parameter for Lookup<B> could not be inferred. So, instead, we make the
`len` method part of a trait without type parameters.

Fixes #19.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant