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
Explicitly define length
, elem
, etc. in Foldable
instance for Data.Functor.Compose
#57
Comments
I think it should be |
Yes, you are right. I edited my proposal to use |
length
in the Foldable
instance for Data.Functor.Compose
length
, etc. in the Foldable
instance for Data.Functor.Compose
length
, etc. in the Foldable
instance for Data.Functor.Compose
length
, elem
, etc. in Foldable
instance for Data.Functor.Compose
On further reflection, I realized that essentially the same situation applies to |
@cdsmith could you please put up a draft MR? |
@Bodigrim Is |
Yes |
I probably cannot put up a draft MR in a timely fashion. I spent some time this evening trying to figure out how to get that checked out and building. Among the things that stumped me were how to get HLS working (it complains about missing a file related to hadrian), how to build just base (instead of rebuilding all of GHC and everything), where to put tests (I can barely find any tests in |
@cdsmith you do not even need to build GHC yourself, CI will do it for you; and at this stage tests are not obligatory. |
@cdsmith unless there is a progress within two weeks, I'll close this as abandoned. If anyone else has bandwidth to create an MR, feel free to do so. |
I will try to work on this over the weekend. It would be a shame to lose what seems like a clear improvement, and I should get better at working in the GHC tree anyway. |
@cdsmith ping. Feel free to advertise this job on irc / twitter / discourse / reddit, but if there is no MR very soon, I'll close. core-libraries-committee/PROPOSALS.md Lines 54 to 57 in dcaf3f2
|
Is there a more traditional issue tracker for base somewhere where I can file this, then? I'm not trying to force anyone else to implement anything, but if this were any other library, I'd think it's worth having an open issue. I cannot seem to find such a thing for base. |
Dunno, you can probably open an issue at GHC bug tracker, linking this discussion. What's your main friction point here? You can copy the source of |
I posted a call for help: https://www.reddit.com/r/haskell/comments/xec5xj/explicitly_define_length_elem_etc_in_foldable/ |
I can take a stab at implementing this. |
@ramirez7 please go ahead with an MR. |
Here is the MR. It is the code in this proposal verbatim. @cdsmith I had never made any modifications to GHC before (only built forks from source), so I wanted to share what I learned that made developing this change in-repo easy:
|
@ramirez7 Thank you so much! I had intended to get to this, but it was becoming a source of a lot of stress for me, so you have no idea how grateful I am that you checked it off my list for me. :) |
Good question. It's very hard to tell what the ramification are! (Regardless of the fate of |
I think I could go either way on the |
Do I think |
My point is: don't spend effort on (FWIW, |
Is there anything wrong with @ramirez7's implementations? If not, then there's not really any effort remaining to be spent, and there's definitely an opportunity to benefit with, for example, |
I tend to agree with @cdsmith here: since @ramirez7 has already spent a considerable effort on implementing partial members of Dear CLC members, before we vote on this, please take a moment to look at the MR: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9001/diffs, as it offers an opportunity for a technical review and nitpicking. @tomjaguarpaw @chessai @cgibbard @mixphix @emilypi |
Sunken-cost fallacy. There is no tests nor benchmarks. Consider There is good foldr f b (Compose fga) = foldr (\ga acc -> foldr f acc ga) b fga
foldr1 f (Compose fga) =
fromMaybe (error "foldr1: empty structure") $
foldr (\ga acc -> if null ga then acc else Just $ let a = foldr1 f ga in maybe a (a `f`) acc) Nothing fga is it really better than the default foldr1 :: (a -> a -> a) -> t a -> a
foldr1 f xs = fromMaybe (errorWithoutStackTrace "foldr1: empty structure")
(foldr mf Nothing xs) Note that inner |
These couldn't have efficient implementations for any container. |
This one seems to have slipped under the radar. It seems we never actually ran a vote! |
I would also like to see some before voting on the MR.
I agree that there's not really a reason to approximate the wheel here, and we should just remove the unnecessary specialised version |
In the interest of reaping some fruits of the long discussion here and as an exception to general practices, I rebased the MR of @ramirez7 in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/10145 and applied suggestions from comments above. I don't have any more time to spend on this, so I'd like to trigger a vote as is. I'm fine if someone votes -1 on the basis of insufficient effort, I'm neither the original proposer nor the original implementer. Dear CLC members, let's finish this up one way or another. The proposal is to define methods of CC @tomjaguarpaw @chshersh @mixphix @parsonsmatt @angerman @hasufell +1 from me. |
So who will take the MR over the finish line? |
If MR !10145 gets approved, I’ll take it over the finish line myself. The only thing it lacks is a changelog. |
In that case I'm +1 |
+1 |
+1 Thanks, @Bodigrim, for pushing the proposal over the finish line! 👏🏻 In the future, I suggest to label proposals as and close them if the original author doesn't have the capacity for implementing the MR and steering the proposal till the end. I don't think that implementation of proposals should be a CLC responsibility. I'm okay to make this one an exception because a lot of work already was put into it and only CHANGELOG entry is required (and Andrew kindly offered his valuable time to finish the PR). |
+1 |
Thanks all, 5 votes out of 7 are enough for approval. |
Explicitly define length, elem, etc. in Foldable instance for Data.Functor.Compose Implementation of haskell/core-libraries-committee#57
Explicitly define length, elem, etc. in Foldable instance for Data.Functor.Compose Implementation of haskell/core-libraries-committee#57
I'm trying to summarise the state of this proposal as part of my volunteering effort to track the progress of all
Please, let me know if you find any mistakes 🙂 |
Currently, the
Foldable
instance forData.Functor.Compose
looks like this:All other methods besides
foldMap
get default implementations, which delegate tofoldMap
. I propose that the following should be added:The inner container in a
Compose
may have a more efficient implementation for other methods of Foldable. In case it does, that should be used instead of falling back tofoldMap
as happens now.This applies to all members of
Foldable
, which presumably are in the class precisely because they can have more efficient implementations. It seems particularly troublesome forlength
andelem
, which are likely to have much more efficient implementations for commonly used containers.The text was updated successfully, but these errors were encountered: