Verify the late-failing implementation of lookup #188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
in issue haskell/containers#794 and PR haskell/containers#800 an alternative implementation of `lookup` is proposed which checks if the key is as expected only when reaching the leaf. This means less branching in the success case or when the lookup key shares long prefixes with existing values, but may do more work when the lookup key is far away from existing keys. Not saying that it’s particularly doubtful that the changed code is correct, but since we have the machinery, why not verify it? So here we go, and yes, it goes through. I don’t think we need to merge the PR like this, and can probably wait for the code to reach a containers release, and then include it there. Also, this PR currently contains a bunch of local hacks that I used to get going again (now that I use NixOS), which need to be removed, merged separately, or made obsolete by better changes to the setup.
|
haskell/containers#800 got merged, so maybe we should merge this too |
|
Ah, maybe not: As I said in the PR description, we should just update the version of containers that we verify |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
in issue haskell/containers#794 and PR
haskell/containers#800 an alternative
implementation of
lookupis proposed which checks if the key is asexpected only when reaching the leaf. This means less branching in the
success case or when the lookup key shares long prefixes with existing
values, but may do more work when the lookup key is far away from
existing keys.
Not saying that it’s particularly doubtful that the changed code is
correct, but since we have the machinery, why not verify it? So here we
go, and yes, it goes through.
I don’t think we need to merge the PR like this, and can probably wait
for the code to reach a containers release, and then include it there.
Also, this PR currently contains a bunch of local hacks that I used to
get going again (now that I use NixOS), which need to be removed, merged
separately, or made obsolete by better changes to the setup.