andraskovacs / dynamic-mvector Goto Github PK

Looking at the code of readBack

-- | Read the back value.  Throws an error if the vector is empty.
readBack :: PrimMonad m => MVector (PrimState m) a -> m a
readBack (MVector v) = do
    MVectorData s v <- readMutVar v
    if (s <= 0) then
        error "Data.Vector.Mutable.Dynamic: reading the back of an empty vector"
    else
        MV.unsafeRead v (MV.length v - 1)
{-# INLINABLE readBack #-}

Shouldn't last line of code be MV.unsafeRead v (s-1)?

Today, new and safeNew return a vector of length equal to its capacity, but the notion of length is the count of valid elements so it should be zero in this case because elements are uninitialized.

I think it is more in line with what implementations of the C++ STL do... but I agree that it is debatable.

My view is that I prefer that the capacity is kept to not waste time reallocating when the vector grows again.

I created unit tests for the few functions I use:

https://github.com/OlivierSohn/hamazed/blob/master/test/Test/Vector.hs

It's testing this reduced version, adapted to support Unboxed vectors and where a sort function was added:

https://github.com/OlivierSohn/hamazed/blob/master/src/Render/Delta/DynUnboxedVec.hs

I think this package would benefit from implementing that kind of tests, both to document the behaviour and to test against regressions.

The problem is that the when guard to check if we need to shrink the vector is also guarding the writeMutVar invocation that decrements the size of the vector. Instead, the size needs to be decremented unconditionally.