Move slice to Partial, add sliceMaybe function #201
Conversation
| import qualified RIO.Vector.Boxed as VB | ||
|
|
||
| spec :: Spec | ||
| spec = |
There was a problem hiding this comment.
Some quickchecks/property tests would be a great addition here.
There was a problem hiding this comment.
Ah, I didn't write any because I didn't see any in ListSpec.hs.
There was a problem hiding this comment.
I was about to replace all the tests with:
spec :: Spec
spec =
describe "sliceMaybe" $ do
prop "is consistent with `slice` (pathological cases)" $
\(QC.Large i) (QC.Large n) v
-> sliceProp i n (DV.fromList v)
prop "is consistent with `slice` (less pathological cases)" $
\(QC.NonNegative i) (QC.NonNegative n) (QC.NonEmpty v)
-> sliceProp i n (DV.fromList v)
where
sliceProp :: Int -> Int -> DV.Vector Char -> QC.Property
sliceProp i n v = QC.withMaxSuccess 10000 $ case VB.sliceMaybe i n v of
Just v' -> DV.slice i n v `shouldBe` v'
Nothing -> evaluate (DV.slice i n v) `shouldThrow` anyException(The second property could be skipped since it is implicit in the first and the number of cases could be reduced to the default of 100, but the coverage may not be extensive.)
However, these tests resulted in:
rio > RIO.Vector
rio > sliceMaybe
rio > is consistent with `slice` (pathological cases) FAILED [1]
rio > is consistent with `slice` (less pathological cases)
rio > +++ OK, passed 10000 tests.
rio >
rio > Failures:
rio >
rio > test/RIO/VectorSpec.hs:23:18:
rio > 1) RIO.Vector.sliceMaybe is consistent with `slice` (pathological cases)
rio > Falsifiable (after 82 tests and 25 shrinks):
rio > Large {getLarge = 2104973872246037021}
rio > Large {getLarge = 7118398164608738787}
rio > ""
rio > did not get expected exception: SomeException
Further investigation led me to open https://gitlab.haskell.org/ghc/ghc/issues/17233.
There was a problem hiding this comment.
What makes you think this is a GHC bug instead of a vector library bug?
There was a problem hiding this comment.
I just used the heuristic that if it is a segfault or the error message says Please report this as a GHC bug it should be reported here. But it may well be (probably is?) a vector bug so I reported it as haskell/vector#257 too.
Special casing is required due to GHC issue <https://gitlab.haskell.org/ghc/ghc/issues/17233>.
| -> Int -- ^ @n@ length | ||
| -> v a | ||
| -> v a | ||
| slice i n v = if i > 0 && n > 0 && i + n < 0 -- `i+n` overflows |
There was a problem hiding this comment.
I'd rather see a patch like this land in vector than here. I'm also concerned about surprising behavior of error types changing.
There was a problem hiding this comment.
OK, I will remove 8673eb7 then.
|
|
||
| spec :: Spec | ||
| spec = | ||
| describe "sliceMaybe" $ do |
There was a problem hiding this comment.
I don't think it's necessary to replace the unit tests with property tests, I was just hoping to include property tests as well.
There was a problem hiding this comment.
That was understood. However, I felt the property test was more to the point and completely specified the intended behavior of sliceMaybe (in terms of slice), thereby making the unit tests redundant.
Things became a bit complicated with the revealed bug of course.
| guard $ i >= 0 | ||
| guard $ n >= 0 | ||
| guard $ Data.Vector.Generic.length v - n >= i | ||
| pure $ Data.Vector.Generic.slice i n v |
There was a problem hiding this comment.
Since we're doing the checking ourselves, moving to unsafeSlice is a good idea.
|
|
Closes #200.