Make CScript (and prevector) c++11 movable. #9349

pull sipa wants to merge 1 commits into bitcoin:master from sipa:movescript changing 3 files +29 −2

sipa commented at 6:16 PM on December 14, 2016: member

Such moves are used when reallocating vectors that contain them, and could be used for more things later.

This change avoids an extra allocation when the data is not inlined.
JeremyRubin commented at 9:16 PM on December 14, 2016: contributor
utAck 7ad1126

minor nits:
- is it necessary to kill the CScript copy constructor in this pull?
- you have an #include "assert.h" in prevector.h that seems to be unused.
sipa commented at 9:39 PM on December 14, 2016: member

is it necessary to kill the CScript copy constructor in this pull?

Yes, the presence of a custom copy constructor in CScript prevents the creation of a default move constructor or move assignment operator. I can add those as well, but given that the default copy constructor is perfectly appropriate, it's better to just pick that one, and get all default constructors and assignment operators.

you have an #include "assert.h" in prevector.h that seems to be unused.

Good catch.
in src/test/prevector_tests.cpp:None in 7ad1126215 outdated
```
 258 |                  test.swap();
 259 |              }
 260 | +            if (((r >> 15) % 16) == 8) {
 261 | +                test.copy();
 262 | +            }
 263 | +            if (((r >> 15) % 32) == 18) {
```
dcousens commented at 10:57 PM on December 14, 2016:

trivial question, but what are these values representing?

sipa commented at 9:05 PM on December 16, 2016:

Just things that are tested with certain percentage chance. The numbers here are chosen so that the copy/move/swap never occur at once (but it hardly matters).

dcousens commented at 11:39 PM on December 27, 2016:

[still trivial] @sipa any reason not to document those percentages? Or perhaps represent them in a non-bit shifting format? I find this to be the case often throughout, and I'm not the biggest fan of having to spend review time on determining the bit shifts in my head compared to higher level issues.

Trivial, but, prolific enough I thought I'd ask.
dcousens approved
fanquake added the label Refactoring on Dec 15, 2016
jonasschnelli approved
jonasschnelli commented at 8:06 AM on December 15, 2016: contributor
Code Review ACK 7ad1126215c0a527182840b2a98e5f83ddfad880.

nits:
- new assert.h include can be removed
- missing test for CScript move

in src/prevector.h:None in 7ad1126215 outdated

 267 | @@ -263,6 +268,10 @@ class prevector {
 268 |          return *this;
 269 |      }
 270 |  
 271 | +    prevector& operator=(prevector<N, T, Size, Diff>&& other) {
 272 | +        swap(other);

gmaxwell commented at 7:51 AM on December 16, 2016:

no return statement in function returning non-void.

sipa commented at 9:03 PM on December 16, 2016:

Fixed.

sipa force-pushed on Dec 16, 2016
sipa commented at 9:04 PM on December 16, 2016: member

@jonasschnelli Fair enough, there is no explicit test for CScript move, but I think we can rely on the C++ default constructors being correctly implemented in the compiler...
theuni commented at 9:34 PM on December 16, 2016: member

utACK other than the nits, looks familiar :)
gmaxwell commented at 9:48 PM on December 17, 2016: contributor

tested ACK.
in src/prevector.h:None in efc0c4cadc outdated
```
   7 | @@ -8,6 +8,7 @@
   8 |  #include <stdlib.h>
   9 |  #include <stdint.h>
  10 |  #include <string.h>
  11 | +#include <assert.h>
```
laanwj commented at 7:45 AM on December 19, 2016:

Does assert.h need to be included here? This doesn't seem to add any assertions (ah, I see @jonasschnelli also mentions this).

sipa commented at 2:29 AM on December 22, 2016:

Good point, fixed.
laanwj commented at 7:48 AM on December 19, 2016: member

utACK efc0c4c, but see the nit

Make CScript (and prevector) c++11 movable.

Such moves are used when reallocating vectors that contain them,
for example.

2ddfcfd2d6

sipa force-pushed on Dec 22, 2016
sipa commented at 2:29 AM on December 22, 2016: member

Nit fixed.
sipa merged this on Dec 27, 2016
sipa closed this on Dec 27, 2016
sipa referenced this in commit 2db4cbcc43 on Dec 27, 2016
codablock referenced this in commit cbbab00c56 on Jan 18, 2018
andvgal referenced this in commit 0b3b3193a5 on Jan 6, 2019
CryptoCentric referenced this in commit 09b4290b06 on Feb 26, 2019
Fuzzbawls referenced this in commit 8dfc4806f7 on May 19, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels