More ergonomic slicing #252
Merged
Conversation
This file contains hidden or 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
|
I'm leaving this as-is for now, as I'd like to take some time to reflect on this new API. @mulimoen, if you want, I'd very much appreciate your feedback on these changes. |
mulimoen
requested changes
Dec 10, 2020
There was a problem hiding this comment.
@vbarrielle I think this API is very nice to have, and makes sense to include. I don't think I would change anything apart from the nitpicks noted here.
You should do a rebase on current master, the clippy changes I've made will probably give merge conflicts
This was me seeing a single missed type disguise, fixed in #258
vbarrielle
added a commit
that referenced
this pull request
Dec 18, 2020
As discussed with @mulimoen in #252 (comment) there needs to be a consistent handling of reborrowing throughout the crate. The chosen pattern is the one implemented here: implement on the base method, calling the actual implementation that's defined on the view type, with the appropriate lifetime.
This trait will allow more ergonomic slicing APIs
To be able to extend the lifetime of the `middle_slice` method targeting `CsMatViewI` without having to use an other name, I'm experimenting a new approach, where I re-implement the method for each main variant. Of course I'd still like to be able to write a single implementation, but I guess only a future version of rust, with specialization, or maybe associated type constructors, will be able to help.
There is no need to return an option as the value 0 is the correct one when no bound is present.
As discussed with @mulimoen in #252 (comment) there needs to be a consistent handling of reborrowing throughout the crate. The chosen pattern is the one implemented here: implement on the base method, calling the actual implementation that's defined on the view type, with the appropriate lifetime.
|
@mulimoen the latest changes should address all your remarks. I've also added the policy on reborrowing on the guidelines to have a point of reference |
mulimoen
approved these changes
Dec 19, 2020
|
Excellent work @vbarrielle! |
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.
As mentionned in #250, the slicing of middle matrices along the outer dimension is quite cumbersome to use, which is particularly visible in the matrix product implementation in
smmp.rs. This PR tries to propose a more ergonomic API.