fix(matrix): respect the documented bounds in set_slice, slice and transpose - #845
Merged
Merged
Conversation
…anspose set_slice is documented to copy only what fits when the slice goes outside the matrix, but a position past the edge underflowed and panicked. It now copies nothing in that case. slice returns WrongIndex for ranges running past the matrix, but a range ending before it starts underflowed and panicked with a capacity overflow. It now returns WrongIndex for that as well. transpose had no check for a matrix without rows, which transposed refuses because the result would have empty rows. Debug builds underflowed, and release builds produced such a matrix. It now panics with the same message as transposed. constrain negated negative values, and -isize::MIN does not fit in an isize, so debug builds panicked on it. It now uses unsigned_abs. Fixes evenfurther#844
Member
|
Good catch |
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
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.
Fixes #844.
Four places where
Matrixpanicked on inputs its documentation covers.set_sliceis documented to copy only the cells that fit when the slice goes outside the matrix, but a position past the edge underflowedself.rows - rowand panicked. It now usessaturating_sub, and copies nothing when nothing fits.slicealready returnsWrongIndexfor a range running past the matrix. A range ending before it starts underflowedrows.end - rows.startand panicked with a capacity overflow; it now getsWrongIndextoo, and the documentation says so.transpose, in place, now refuses a matrix without rows with exactly the check and messagetransposedalready uses, and documents it under# Panics. Before, debug builds underflowed and release builds left a matrix with rows of zero columns, whichMatrixotherwise never allows. Nothing inside the crate callstranspose; the internal callers usetransposed, which already had this check.utils::constrain, behindMatrix::constrainandGrid::constrain, negated negative values, and-isize::MINdoes not fit in anisize. It now usesunsigned_abs, which gives the same result for every other value.Tests
In
tests/matrix.rsandtests/utils.rs:set_slice_outside_the_matrix_changes_nothing, including a position partly inside to check that clipping still copies what fitsslice_with_a_reversed_range_is_an_error, for both rows and columnstranspose_in_place_refuses_what_transposed_refuses, matching the panic messageconstrain_the_most_negative_value, againstrem_euclidEach fails on current
main; theconstrainone in a debug build, since release happened to wrap to the right answer.For the rest of
MatrixI compared rotations in both directions for every count, both flips,transposedand the in-placetranspose(including the non-square cycle-following path),slice,neighbours,in_directionandconstrainagainst straightforward reimplementations on a few hundred random matrices up to 6x6. They all agreed, so these fixes are limited to the edges listed above.