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.

Sign up for GitHub

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

Jump to bottom

Tidy up HashMap and MultiMap #864

Closed
wants to merge 1 commit into from
Closed

Tidy up HashMap and MultiMap #864

wants to merge 1 commit into from
+26 −51

Conversation

fumieval
Copy link
Contributor

Removed a lot of unnecessary strict bindings

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@kazu-yamamoto
Copy link
Contributor

@fumieval Just a confirmation. Do you notice that warp.cabal has Strict and StrictData?

@fumieval
Copy link
Contributor Author

No, but that shouldn't matter for this change

kazu-yamamoto
kazu-yamamoto reviewed Nov 11, 2021
View reviewed changes
warp/Network/Wai/Handler/Warp/MultiMap.hs Outdated
!h = hash path
!mm' = I.insertWith (<>) h [(path,v)] mm
insert path v (MultiMap mm) = MultiMap
$ I.insertWith (flip (<>)) (hash path) [(path,v)] mm
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why flip is introduced here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

insertWith (++) is quadratic because it appends an element to a list. Does anything depend on the order of inner lists?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't insertWith (++) prepending?
https://hackage.haskell.org/package/containers-0.6.5.1/docs/Data-IntMap-Strict.html#v:insertWith

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, I misunderstood then

warp/Network/Wai/Handler/Warp/MultiMap.hs Outdated
[] -> go kss acc
_ -> go kss ((h,rs) : acc)
pruneWith (MultiMap mm) action = I.foldrWithKey
(\h s cont acc -> do
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to change this function to a local function after where.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain why? Is it a style preference or does it produce a better core?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a style preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@fumieval
Tidy up HashMap and MultiMap
9d6a7ce 
Removed a lot of unnecessary strict bindings
@kazu-yamamoto
Copy link
Contributor

I run CI again.

@kazu-yamamoto
Copy link
Contributor

We should understand whether or not the following failure on Windows is due to this PR:

warp                > 
warp                >   test\RunSpec.hs:322:9: 
warp                >   1) Run, chunked bodies, in chunks
warp                >        uncaught exception: IOException of type InvalidArgument
warp                >        Network.Socket.sendBuf: invalid argument (Invalid argument)
warp                > 
warp                >   To rerun use: --match "/Run/chunked bodies/in chunks/"
warp                > 
warp                > Randomized with seed 1880375916
warp                > 
warp                > Finished in 7.9339 seconds
warp                > 96 examples, 1 failure
warp                > Test suite spec failed

@kazu-yamamoto
Copy link
Contributor

I run CI once more and it succeeded. Let's merge.

kazu-yamamoto
kazu-yamamoto approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

Now LGTM.

kazu-yamamoto added a commit to kazu-yamamoto/wai that referenced this pull request Nov 11, 2021
@kazu-yamamoto
Merge PR yesodweb#864
0244135
@kazu-yamamoto
Copy link
Contributor

Rebased and merged. Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kazu-yamamoto kazu-yamamoto kazu-yamamoto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@fumieval @kazu-yamamoto