prevector: fix 2 bugs in currently unreached code paths #7888

pull kazcw wants to merge 3 commits into bitcoin:master from kazcw:pvfix changing 2 files +18 −16

kazcw commented at 2:58 AM on April 16, 2016: contributor

It looks like maybe this swap method is from an earlier iteration of prevector that used the LSB of size as a "direct/indirect" tag. The bad path isn't ever hit because in all current instances of swapping two prevectors, one is newly value-initialized and thus has an even size (0).
laanwj commented at 5:19 AM on April 16, 2016: member

Concept ACK. I think we need a unit test that fail before this, and succeed after this.
sipa commented at 9:17 AM on April 16, 2016: member

Nice catch, thanks.

Code review ACK, but agree that a test for this behaviour would be welcome.
kazcw force-pushed on Apr 16, 2016
kazcw force-pushed on Apr 16, 2016

prevector: destroy elements only via erase()

Fixes a bug in which pop_back did not call the deleted item's destructor.

Using the most general erase() implementation to implement all the others
prevents similar bugs because the coupling between deallocation and destructor
invocation only needs to be maintained in one place.
Also reduces duplication of complex memmove logic.

1e2c29f263

test prevector::swap

- add a swap operation to prevector tests (fails due to broken prevector::swap)
- fix 2 prevector test operation conditions that were impossible

4ed41a2b61

prevector::swap: fix (unreached) data corruption

swap was using an incorrect condition to determine when to apply an optimization
(not swapping the full direct[] when swapping two indirect prevectors).

Rather than correct the optimization I'm removing it for simplicity. Removing
this optimization minutely improves performance in the typical (currently only)
usage of member swap(), which is swapping with a freshly value-initialized
object.

a7af72a697

kazcw force-pushed on Apr 16, 2016
laanwj added the label Utils and libraries on Apr 18, 2016
laanwj commented at 10:41 AM on April 18, 2016: member

Thanks for adding the test. I verified that it passes with, and fails without a7af72a. tACK a7af72a
laanwj merged this on Apr 18, 2016
laanwj closed this on Apr 18, 2016
laanwj referenced this in commit ec870e1399 on Apr 18, 2016
kazcw deleted the branch on Apr 22, 2016
sickpig referenced this in commit 8992e345fe on Mar 9, 2018
Fuzzbawls referenced this in commit 8dfc4806f7 on May 19, 2020
zkbot referenced this in commit cd00fb08d5 on Feb 17, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels