limitedmap fixes and tests #6561

Merged
merged 3 commits into from Aug 19, 2015

Conversation

Projects
None yet
5 participants
@casey
Contributor

casey commented Aug 17, 2015

This PR consists of three commits.

The first fixes a bug where the size of a limited map was actually one less than the maximum size set. This shouldn't matter at all for the existing code, since there is only one limitedmap, and its size is very large.

The second disallows unlimited limited maps, since that doesn't make very much sense, and simplifies the code a little bit. (I'm not too attached to this, in case anyone doesn't like this change I can just remove it.)

The third adds unit tests for limitedmap, which were previously absent.

I elected to leave the commits unsquashed for review, since they're logically separate, but I can squash if desired.

@casey casey changed the title from Limitedmap fixes to limitedmap fixes and tests Aug 17, 2015

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 17, 2015

Member

ACK

Member

sipa commented Aug 17, 2015

ACK

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Aug 17, 2015

Contributor

utACK

Contributor

TheBlueMatt commented Aug 17, 2015

utACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 19, 2015

Member

utACK. Thanks a lot for adding tests!

Member

laanwj commented Aug 19, 2015

utACK. Thanks a lot for adding tests!

@laanwj laanwj merged commit 7bd57bb into bitcoin:master Aug 19, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 19, 2015

Merge pull request #6561
7bd57bb Add limitedmap test (Casey Rodarmor)
8b06894 Disallow unlimited limited maps (Casey Rodarmor)
fd2d862 Make limited map actually respect max size (Casey Rodarmor)

@casey casey deleted the casey:limitedmap branch Aug 19, 2015

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Aug 20, 2015

Contributor

Is it worth adding a test to verify that insert after max_size is reached, it doesn't increment size?
(Happy to do this)

Contributor

dcousens commented Aug 20, 2015

Is it worth adding a test to verify that insert after max_size is reached, it doesn't increment size?
(Happy to do this)

@casey

This comment has been minimized.

Show comment
Hide comment
@casey

casey Aug 20, 2015

Contributor

It does this already, unless you were thinking of a more substantial test: 11 elements are added (one one off, and then ten in a loop) and then line 39 checks that there are only 10 elements.

Contributor

casey commented Aug 20, 2015

It does this already, unless you were thinking of a more substantial test: 11 elements are added (one one off, and then ten in a loop) and then line 39 checks that there are only 10 elements.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Aug 20, 2015

Contributor

Sorry, I missed that first insert @casey. utACK :)

Contributor

dcousens commented Aug 20, 2015

Sorry, I missed that first insert @casey. utACK :)

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 9, 2016

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 9, 2016

Add limitedmap test
Github-Pull: #6561
Rebased-From: 7bd57bb

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016

@str4d str4d referenced this pull request in zcash/zcash Jul 14, 2017

Open

Bitcoin 0.12 P2P/Net PRs 1 #2534

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