Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unsafeToArray function for getting the underlying boxed Array #434

Merged
merged 2 commits into from
May 23, 2022

Conversation

lehins
Copy link
Contributor

@lehins lehins commented May 22, 2022

#245 requested access to Vector constructor. There are a few other requests like that, see this #357 (comment)

However, there is a universal agreement that ability to construct a very unsafe Vector with a regular type constructor is unsettling to say the least. See this: #245 (comment) and this #49 (comment) Let's be honest, creating an Internal module just for constructors is not going to happen and separating all unsafe functions into Unsafe module is lost cause too, see #361

The next best thing we can do, I think, is to create a couple of unsafe* functions that let users create/access underlying buffers in the unsafe way, similar to unsafeToForeignPtr.

This PR implements what I suggest, but only for immutable boxed vector. Mutable vector constructor have been exported since inception, so might as well leave it be.

@chessai
Copy link
Member

chessai commented May 22, 2022

This looks good, but I've always wanted the following in vector as well:

-- performs a copy, for when I no longer need slicing
toArray :: Vector a -> Array a

-- provided by this PR, not sure what warrants "unsafe". surely using the result of this function would be potentially unsafe, but this function alone shouldn't be, as far as I can tell
unsafeToArray :: Vector a -> (Array a, Int, Int)

-- construct a vector with offset 0 and length equal to the array
fromArray :: Array a -> Vector a

-- unsafe because of manual slicing
unsafeFromArray :: Array a -> Int -> Int -> Vector a

@Shimuuar
Copy link
Contributor

Indeed, there's nothing unsafe with unsafeToArray. Only construction is unsafe. And we may add constructor in both unsafe and variety

-- Throws exception if slice is invalid
fromArraySlice :: Array a -> Int -> Int -> Vector a
-- unsafe because of manual slicing
unsafeFromArraySlice :: Array a -> Int -> Int -> Vector a

@lehins
Copy link
Contributor Author

lehins commented May 23, 2022

not sure what warrants "unsafe". surely using the result of this function would be potentially unsafe, but this function alone shouldn't be, as far as I can tell

This is very good point. I'll rename it to toArraySlice

-- performs a copy, for when I no longer need slicing
toArray :: Vector a -> Array a

-- construct a vector with offset 0 and length equal to the array
fromArray :: Array a -> Vector a

These two functions have been added in 0.12.2

-- unsafe because of manual slicing
unsafeFromArray :: Array a -> Int -> Int -> Vector a

I'll add it as well, except for consistency I'll name it unsafeFromArraySlice as @Shimuuar suggested.

-- Throws exception if slice is invalid
fromArraySlice :: Array a -> Int -> Int -> Vector a

I don't want to add another partial function like that. It is always possible to achieve the same with slice i n . fromArray

@lehins lehins force-pushed the access-to-vector-internal branch from 10d5ba1 to 571356a Compare May 23, 2022 10:41
@lehins lehins force-pushed the access-to-vector-internal branch from 571356a to 9ed2f75 Compare May 23, 2022 10:44
@lehins
Copy link
Contributor Author

lehins commented May 23, 2022

Addressed all the feedback. This PR is ready for a followup review.

@lehins lehins requested a review from Shimuuar May 23, 2022 11:00
Copy link
Contributor

@Shimuuar Shimuuar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Another option is add read-only pattern like one below. It allows to look at vector internals but doesn't allow construction.

pattern Vector :: forall a. Int -> Int -> Array a -> Vector a
pattern Vector off sz arr <- MkVector off sz arr
{-# COMPLETE Vec #-}

@lehins lehins merged commit afb189a into master May 23, 2022
@lehins lehins deleted the access-to-vector-internal branch June 19, 2022 12:19
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Aug 19, 2022
# Changes in version 0.13.0.0

 * `mkType` from `Data.Vector.Generic` is deprecated in favor of
   `Data.Data.mkNoRepType`
 * The role signatures on several `Vector` types were too permissive, so they
   have been tightened up:
   * The role signature for `Data.Vector.Mutable.MVector` is now
     `type role MVector nominal representational` (previously, both arguments
     were `phantom`). [#224](haskell/vector#224)
   * The role signature for `Data.Vector.Primitive.Vector` is now
     `type role Vector nominal` (previously, it was `phantom`).
     The role signature for `Data.Vector.Primitive.Mutable.MVector` is now
     `type role MVector nominal nominal` (previously, both arguments were
     `phantom`). [#316](haskell/vector#316)
   * The role signature for `Data.Vector.Storable.Vector` is now
     `type role Vector nominal` (previous, it was `phantom`), and the signature
     for `Data.Vector.Storable.Mutable.MVector` is now
     `type role MVector nominal nominal` (previous, both arguments were
     `phantom`). [#235](haskell/vector#235)

     We pick `nominal` for the role of the last argument instead of
     `representational` since the internal structure of a `Storable` vector is
     determined by the `Storable` instance of the element type, and it is not
     guaranteed that the `Storable` instances between two representationally
     equal types will preserve this internal structure.  One consequence of this
     choice is that it is no longer possible to `coerce` between
     `Storable.Vector a` and `Storable.Vector b` if `a` and `b` are nominally
     distinct but representationally equal types. We now provide
     `unsafeCoerce{M}Vector` and `unsafeCast` functions to allow this (the onus
     is on the user to ensure that no `Storable` invariants are broken when
     using these functions).
 * Methods of type classes `Data.Vector.Generic.Mutable.MVector` and
   `Data.Vector.Generic.Vector` use concrete monads (`ST`, etc) istead of being
   polymorphic (`PrimMonad`, etc). [#335](haskell/vector#335).
   This makes it possible to derive `Unbox` with:
   * `GeneralizedNewtypeDeriving`
   * via `UnboxViaPrim` and `Prim` instance
   * via `As` and `IsoUnbox` instance: [#378](haskell/vector#378)
 * Add `MonadFix` instance for boxed vectors: [#312](haskell/vector#312)
 * Re-export `PrimMonad` and `RealWorld` from mutable vectors:
   [#320](haskell/vector#320)
 * Add `maximumOn` and `minimumOn`: [#356](haskell/vector#356)
 * The functions `scanl1`, `scanl1'`, `scanr1`, and `scanr1'` for immutable
   vectors are now defined when given empty vectors as arguments,
   in which case they return empty vectors. This new behavior is consistent
   with the one of the corresponding functions in `Data.List`.
   Prior to this change, applying an empty vector to any of those functions
   resulted in an error. This change was introduced in:
   [#382](haskell/vector#382)
 * Change allocation strategy for `unfoldrN`: [#387](haskell/vector#387)
 * Remove `CPP` driven error reporting in favor of `HasCallStack`:
   [#397](haskell/vector#397)
 * Remove redundant `Storable` constraints on to/from `ForeignPtr` conversions:
   [#394](haskell/vector#394)
 * Add `unsafeCast` to `Primitive` vectors: [#401](haskell/vector#401)
 * Make `(!?)` operator strict: [#402](haskell/vector#402)
 * Add `readMaybe`: [#425](haskell/vector#425)
 * Add `groupBy` and `group` for `Data.Vector.Generic` and the specialized
   version in `Data.Vector`, `Data.Vector.Unboxed`, `Data.Vector.Storable` and
   `Data.Vector.Primitive`. [#427](haskell/vector#427)
 * Add `toArraySlice` and `unsafeFromArraySlice` functions for conversion to and
   from the underlying boxed `Array`: [#434](haskell/vector#434)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants