Implement generic index types #19
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a version of generic index types that compiles, but it has still many rough edges. A few examples: - `CustomIndexing` trick in traits.rs, which is needed for `impl ... for &G`, because using just `Self` causes an (understandable) compilation error. If there is no other way, it should at least get a better name (`DelegateIndexing`?) and be private? - In many cases there is a "VertexRef::data type annotations needed" (`VertexRef::data` is just one example) error. This is very unergonomic and a solution must be found. This arises from the fact that plain tuples do not hold information about `Ix: Indexing`. - During the implementation, clones were deliberately added anywhere where needed to make code compile. They should be reviewed if some of them can be removed by changing an API (e.g., take something by reference) - `UseIndex` trait in `visit::raw` is quite an overkill. Maybe it's a necessary implementation detail (we need `Ix: Indexing` to avoid "Ix is not constrained by trait, Self type, ..." error), but it should not be visible outside of `visit::raw`. - Conversions from/to usize for `NumIndexType` are cumbersome right now as explicit `NumIndexType::from_usize` must be used instead of simple `.into()`. This should be improved as well. Following commits should resolve mentioned problems and polish the code in general.
|
Implements #17 |
This is just to be able to make a comment in the future about why this workaround is needed (or make it easier to find all occurrences if a better way is found).
This allows use of owned indices with high-level API for graphs from `graph` module, if the index types are Copy, which they will be in the vast maority of cases. Using owned indices is kind of a convention in Rust's graph ecosystem. This technique is not employed in the base traits as that would complicate the overall interface and result in more code bloat during monomorphization.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a version of generic index types that compiles, but it has still many rough edges. A few examples:
CustomIndexingtrick in traits.rs, which is needed forimpl ... for &G, because using justSelfcauses an (understandable) compilation error. If there is no other way, it should at least get a better name (DelegateIndexing?) and be private?In many cases there is a "VertexRef::data type annotations needed" (VertexRef::datais just one example) error. This is very unergonomic and a solution must be found. This arises from the fact that plain tuples do not hold information aboutIx: Indexing.During the implementation, clones were deliberately added anywhere where needed to make code compile. They should be reviewed if some of them can be removed by changing an API (e.g., take something by reference)Checked all new clones and I didn't find any good opportunity to avoid a clone (it is either so cheap that it is not worthy to complicate the API or is in testing code).UseIndextrait invisit::rawis quite an overkill. Maybe it's a necessary implementation detail (we needIx: Indexingto avoid "Ix is not constrained by trait, Self type, ..." error), but it should not be visible outside ofvisit::raw.Conversions from/to usize forNumIndexTypeare cumbersome right now as explicitNumIndexType::from_usizemust be used instead of simple.into(). This should be improved as well.Use Borrow trait for index arguments, so that both owned indexes and references to indexes can be used in API such asvertex(...)andadd_edge(...). This will help ergonomics and aesthetics on the user side in majority of cases, when default index types are used as they areCopy.MakeIndexType: DebugFollowing commits should resolve mentioned problems and polish the code in general.