Add MonadFix instance for boxed vectors by Shimuuar · Pull Request #312 · haskell/vector

Shimuuar · 2020-06-06T14:44:04Z

Supersedes #179.
Fixes#178

See both for earlier discussion. LGTM but I never really understood nor used MonadFix

@treeowl Do you still has interest in this PR? And by chance do you remember what does this passage means?

-- It's perfectly safe to use non-monadic indexing within
-- each element, as the result of indexing will be demanded
-- as soon as the vector is produced.

I *believe* this is equivalent to the instance for `[]`. Writing QuickCheck properties for `mfix` seems pretty tricky, so I just added a small unit test. Fixeshaskell#178

treeowl · 2020-06-06T15:16:11Z

Why do we need a new PR? What was wrong with the previous one? I'll try to answer your questions today.

Shimuuar · 2020-06-06T15:19:42Z

I resolved merge conflicts and I can't push to your repo.

P.S. If you want to amend anything in this PR ping me, I'll grant you push permissions in my fork

Shimuuar · 2020-06-06T15:23:19Z

P.P.S reopened #179 just in case

treeowl · 2020-06-06T19:28:12Z

+ -- itself. These measures should prevent memory leaks.
+ -- It's perfectly safe to use non-monadic indexing within
+ -- each element, as the result of indexing will be demanded
+ -- as soon as the vector is produced.


What I mean about this is that the use of (!) in line 399 is okay: it can't cause a vector to be kept alive by a thunk. Why? Because we don't create that vector until we need the value of the result of that indexing operation. This is as opposed to the headM we need to get h, where we'd otherwise keep v0 alive in the first element of the result vector until that first element is forced by someone else.

Thanks!
I'll integrate that explanation into comment tomorrow. Right now it's somewhat cryptic

treeowl · 2020-06-06T19:30:03Z

I see now that you didn't do anything but merge. Yeah, go ahead and squash this when you're ready; the other PR can go away. Would've been better to handle it differently, probably, but now that you've done the merging work there's no point in doing it again.

Shimuuar · 2020-06-06T20:45:17Z

Sorry for the confusion. I should've word PR message better and least mention merge. I assumed that you gave up on PR after all these years

Bodigrim · 2020-06-09T18:43:37Z

 {-# INLINE munzip #-}
 munzip = unzip

+-- | @since 0.13.0.0


It would be nice to comment on the semantics of MonadFix Vector. It is similar to MonadFix [] instance, isn't it? If it is, then what's wrong with definining it as below?
mfix f = V.fromList (mfix (V.toList . f))
I mean, the proposed implementation already conses elements one by one, which is probably not very efficient.

The proposed implementation does not cons elements one by one. It only uses consonce, and that cons should fuse with the following generate. A comment would be in order. Yes, semantics are the same as the [] instance, or close enough.

The listy implementation could go in the instance documentation if you like.

Ah, sorry, my bad, you are right.
Yes, adding the "listy" implementation in the instance documentation sounds good to me.

Shimuuar · 2020-06-09T20:39:42Z

I've updated comments

Shimuuar · 2020-06-10T18:43:08Z

I think now PR is ready for merge. I no one will voice opposition I'll merge it by the end of the week

treeowl · 2020-06-10T19:15:23Z

+instance MonadFix Vector where
+ -- We take care to dispose of v0 as soon as possible (see headM docs).
+ -- We also avoid setting up the result vector to refer to
+ -- itself. These measures should prevent memory leaks.


I no longer know what I meant by "We also avoid setting up the result vector to refer to itself." Perhaps We should just change the above to
-- To prevent memory leaks, we take care to dispose of v0 as soon as possible (see headM docs).

Makes sense. I don't understand what does it mean either

Shimuuar · 2020-06-14T14:16:59Z

Thanks you, everyone. PR is finally merged

It's #179 with merged into latest master and documentation tweaks Originally PR authored by David Feurer I *believe* this is equivalent to the instance for `[]`. Writing QuickCheck properties for `mfix` seems pretty tricky, so I just added a small unit test. Co-authored-by: David Feuer <David.Feuer@gmail.com>

# 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)

treeowland others added 2 commits July 5, 2017 17:23

Add MonadFix instance
915a5bb
I *believe* this is equivalent to the instance for `[]`. Writing QuickCheck properties for `mfix` seems pretty tricky, so I just added a small unit test. Fixeshaskell#178

Merge branch 'master' into monad-fix
f841564

Shimuuar mentioned this pull request Jun 6, 2020
Add MonadFix instance #179
Closed

treeowl reviewed Jun 6, 2020
View reviewed changes

Shimuuar added 4 commits June 9, 2020 21:30

Attempt to make documentation somewhat clearer
746d714

Add changelog entry
4f01752

Add since aanotation
c7bb6d2

Merge branch 'master' into monad-fix
0b43390

Bodigrim reviewed Jun 9, 2020
View reviewed changes

Update comments
9a27eb6

Documentation tweaks
da92174

Bodigrim approved these changes Jun 10, 2020
View reviewed changes

treeowl reviewed Jun 10, 2020
View reviewed changes

Incorporate@treeowl's comment
6dd926c

Shimuuar added this to the 0.13 milestone Jun 11, 2020

Shimuuar merged commit 53d247b into haskell:masterJun 14, 2020

Shimuuar deleted the monad-fix branch June 19, 2020 18:15

Uh oh!

Conversation

Shimuuar commented Jun 6, 2020

Uh oh!

treeowl commented Jun 6, 2020

Uh oh!

Shimuuar commented Jun 6, 2020

Uh oh!

Shimuuar commented Jun 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treeowl commented Jun 6, 2020

Uh oh!

Shimuuar commented Jun 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shimuuar commented Jun 9, 2020

Uh oh!

Shimuuar commented Jun 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shimuuar commented Jun 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants