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 Array.foldlM' and foldlWithKeyM' #219

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ocramz
Copy link

@ocramz ocramz commented Jan 15, 2019

No description provided.

@ocramz
Copy link
Author

ocramz commented Jan 15, 2019

Hi ! I was missing a monadic indexed fold, I hope you like this addition :)

@ocramz
Copy link
Author

ocramz commented Jan 23, 2019

ping!

@treeowl
Copy link
Collaborator

treeowl commented Jan 23, 2019

Sorry, I seem to have missed this. I have some concerns.

If we add a left fold, we will certainly also want to add a right fold, since HashMap doesn't care. I'll assume this change.

If we add these to HashMap, we surely want to add similar ones to HashSet. I'll assume this change as well.

Why a strict fold? In strict monads (the only ones for which this strictness is really meaningful anyway), the user can insert strictness into a lazy fold by supplying a function that's strict in the accumulator. I'm not saying we necessarily shouldn't offer the strict version (there are likely efficiency benefits for certain monads), but offering only that, and not the more fundamental lazy version, seems odd.

Do we get a real performance benefit by implementing this directly rather than using foldrWithKey? If so, that would be a strong reason to add this. If not, I'd really want to see a formal proposal on the libraries list to get community guidance. My bet is that the versions below are just fine.

foldlWithKeyM :: Monad m => (a -> k -> v -> m a) -> a -> HashMap k v -> m a
foldlWithKeyM f a0 hm = foldrWithKey go pure hm a0
  where
    go k v r a = f a k v >>= r
{-# INLINE foldlWithKeyM #-}

foldlWithKeyM' :: Monad m => (a -> k -> v -> m a) -> a -> HashMap k v -> m a
foldlWithKeyM' f a0 hm = foldrWithKey go (pure $!) hm a0
  where
    go k v r !a = f a k v >>= r
{-# INLINE foldlWithKeyM' #-}

@ocramz
Copy link
Author

ocramz commented Jan 24, 2019

Thank you @treeowl for your feedback! I hadn't thought of performance implications to be honest and I'll apply the changes you propose:

  • revert to the non-strict implementations based on foldrWithKey you propose
  • add foldrWithKeyM in HashMap
  • add foldrWithKeyM, foldlWithKeyM, foldlWithKeyM' in HashSet

@treeowl
Copy link
Collaborator

treeowl commented Jan 24, 2019

Oh, silly me... We don't need those lazy versions in HashSet because they're in Data.Foldable. But I'd really like to see a proposal sent to libraries@haskell.org to determine which of these the community thinks are worth adding.

@sjakobi
Copy link
Member

sjakobi commented Jun 24, 2020

Marking as a draft, since this PR doesn't seem to be ready for review.

@sjakobi sjakobi marked this pull request as draft June 24, 2020 11:00
@ocramz
Copy link
Author

ocramz commented Jun 25, 2020

@sjakobi @treeowl I don't have the bandwidth to push this forward for the time being.

@sjakobi
Copy link
Member

sjakobi commented Jun 25, 2020

Thanks for the update, @ocramz!

For future reference, here's a related feature request for containers: haskell/containers#678

Since it's an API extension without an obvious precedent, I agree that it should be discussed on the libraries list.

@sjakobi sjakobi added the abandoned This PR needs a new driver label Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned This PR needs a new driver feature-request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants