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

Change default insertBy implementation to work better with dlists. #18

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

AndreasPK
Copy link
Contributor

Instead of repeatedly computing head share the head computation.
I've also replaced head and tail by a single call to uncons for similar reasons and
replaced the default uncons implementation for DList by one based on it's pattern synonym.

Instead of repeatedly computing head share the `head` computation.
I've also replaced `head` and `tail` by a single call to uncons for
similar reasons.

In the de
@AndreasPK
Copy link
Contributor Author

While looking into https://gitlab.haskell.org/ghc/ghc/-/issues/22425 I noticed this. This helps to pull performance for sort on dlists back to 9.4.2 levels on ghc-9.4.3 and just generally seems better to me independent of the ghc bug.

insertBy cmp x ys =
case uncons ys of
Nothing -> singleton x
Just (ys_head,ys_tail) -> case cmp x ys_head of
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style: no snake-case please. (So far, this code base has a single snake-case id only (elem_by) which sneaked in...)
Please us CamlCase or simple identifiers.

@@ -56,6 +59,11 @@ instance ListLike (DList a) a where
--toList = D.toList
--fromList = D.fromList
replicate = D.replicate
uncons xs = case xs of
D.Nil -> Prelude.Nothing
D.Cons d_head l_tail -> Prelude.Just (d_head,fromList l_tail)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please no snake_case.

Copy link
Collaborator

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM except for style issues.

@ddssff ddssff merged commit 1b11462 into ddssff:master Nov 15, 2022
@ddssff
Copy link
Owner

ddssff commented Nov 15, 2022

I hope I didn't merge this prematurely. Should I leave this repo to you Andreas?

@andreasabel
Copy link
Collaborator

I don't mind co-maintainership, I think it helps keeping repos alive.

A "changes requested" should maybe not be ignored, though. There is branch protection rules that would enforce this, but I am not the repo admin here, I cannot change the settings.

@ddssff
Copy link
Owner

ddssff commented Nov 16, 2022

Ok I will be more careful. I did that very early in the morning.

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.

3 participants