Explicitly initialize prevector _union #14028

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

Merged

laanwj merged 1 commit into bitcoin:master from Empact:prevector-explicit-initialization

Aug 29, 2018

+5 −5

Contributor

Empact commented Aug 23, 2018 •

edited

Loading

No description provided.

Contributor Author

Empact commented Aug 23, 2018

Prompted by #13902

fanquake added the Refactoring label

Empact mentioned this pull request

prevector: Suppress two -Wmaybe-uninitialized (gcc) warnings in indirect_ptr(...) #13902

Closed

Empact force-pushed the prevector-explicit-initialization branch 2 times, most recently from eeea910 to bfa1d03 Compare

August 23, 2018 09:39

Empact commented

View reviewed changes

src/prevector.h Outdated

    
                          indirect.capacity = _size;

                          _size += N + 1;

                          return indirect;

                      }

Contributor Author

Empact Aug 23, 2018

Compare with:

bitcoin/src/prevector.h

Lines 185 to 192 in 271b379

    
           char* new_indirect = static_cast<char*>(malloc(((size_t)sizeof(T)) * new_capacity)); 
        
           assert(new_indirect); 
        
           T* src = direct_ptr(0); 
        
           T* dst = reinterpret_cast<T*>(new_indirect); 
        
           memcpy(dst, src, size() * sizeof(T)); 
        
           _union.indirect = new_indirect; 
        
           _union.capacity = new_capacity; 
        
           _size += N + 1;

DrahtBot mentioned this pull request

Remove ambiguity in construction of prevector #14030

Merged

Contributor

DrahtBot commented Aug 23, 2018 •

edited

Loading

No more conflicts as of last run.

sipa reviewed

View reviewed changes

src/prevector.h Outdated

    
            @@ -248,32 +264,24 @@ class prevector {
          
                  prevector() : _size(0), _union{{}} {}

                  explicit prevector(size_type n) : _size(0) {

                      resize(n);

                  explicit prevector(size_type n) : _size(n), _union{init_union()} {

Member

sipa Aug 23, 2018

This is wrong. The _size field does not directly encode the size (see the comments above the class).

Contributor Author

Empact Aug 23, 2018 •

edited

Loading

init_union adds the N + 1 when appropriate.

Member

sipa Aug 23, 2018

Yuck.

Contributor Author

Empact Aug 23, 2018

Fair enough, moved it out of the initializer.

Empact force-pushed the prevector-explicit-initialization branch 4 times, most recently from 2e2b691 to b8212af Compare

August 23, 2018 17:31

Contributor

practicalswift commented Aug 23, 2018

Concept ACK

This fixes the compiler warning described in #13902

Empact force-pushed the prevector-explicit-initialization branch 2 times, most recently from 63dc8f2 to f955a50 Compare

August 24, 2018 08:53

Contributor Author

Empact commented Aug 24, 2018 •

edited

Loading

Pared the PR down to just explicitly initializing the _union

Contributor

practicalswift commented Aug 24, 2018

The updated version still fixes the compiler warning.

ACK f955a50384ad53e6d1e89f373905ea8dabc46e93

DrahtBot added the Needs rebase label


          Explicitly initialize prevector _union

1d9aa00

Empact force-pushed the prevector-explicit-initialization branch from f955a50 to 1d9aa00 Compare

August 27, 2018 16:50

Contributor Author

Empact commented Aug 27, 2018

Rebased for #14030

Contributor

practicalswift commented Aug 27, 2018

utACK 1d9aa00

1 similar comment

Contributor

ken2812221 commented Aug 27, 2018

utACK 1d9aa00

DrahtBot removed the Needs rebase label

laanwj merged commit 1d9aa00 into bitcoin:master

laanwj added a commit that referenced this pull request


          Merge #14028: Explicitly initialize prevector _union

b0eb8f7

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

Bushstar mentioned this pull request

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

Empact mentioned this pull request

refactor: Lift prevector default vals to the member declaration #14266

Merged

Empact deleted the prevector-explicit-initialization branch

October 9, 2018 05:23

cmihai mentioned this pull request

Fix GCC warnings in prevector.h by initializing class fields dtr-org/unit-e#1047

Merged

maflcko pushed a commit that referenced this pull request


          Merge #14266: refactor: Lift prevector default vals to the member dec…

…laration

d2eee87 Lift prevector default vals to the member declaration (Ben Woosley)

Pull request description:

  I overlooked this possibility in #14028

ACKs for commit d2eee8:
  promag:
    utACK d2eee87, change looks good because members are always initialized.
  251Labs:
    utACK d2eee87 nice one.
  ken2812221:
    utACK d2eee87
  practicalswift:
    utACK d2eee87
  scravy:
    utACK d2eee87

Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request


          Merge bitcoin#14266: refactor: Lift prevector default vals to the mem…

e97c1bc

…ber declaration

d2eee87 Lift prevector default vals to the member declaration (Ben Woosley)

Pull request description:

  I overlooked this possibility in bitcoin#14028

ACKs for commit d2eee8:
  promag:
    utACK d2eee87, change looks good because members are always initialized.
  251Labs:
    utACK d2eee87 nice one.
  ken2812221:
    utACK d2eee87
  practicalswift:
    utACK d2eee87
  scravy:
    utACK d2eee87

Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97

sickpig mentioned this pull request

[port] update prevector.h BitcoinUnlimited/BitcoinUnlimited#2109

Merged

random-zebra mentioned this pull request

[Core] Prevector PIVX-Project/PIVX#1557

Merged

1 task

Fuzzbawls added a commit to PIVX-Project/PIVX that referenced this pull request


          Merge #1557: [Core] Prevector

8dfc480

a8b965e Explicitly initialize prevector _union (random-zebra)
bdd98e8 [Trivial] Add a comment on the use of prevector in script. (random-zebra)
bbaa6e3 Clarify prevector::erase and avoid swap-to-clear (random-zebra)
28a8435 prevector: assert successful allocation (random-zebra)
de41cb5 Only call clear on prevector if it isn't trivially destructible (random-zebra)
8784df3 Make CScript (and prevector) c++11 movable. (random-zebra)
83f9ac6 serialize: Deprecate `begin_ptr` / `end_ptr` (random-zebra)
c3ecc12 prevector: add C++11-like data() method (random-zebra)
5490aa0 test prevector::swap (random-zebra)
035760e prevector::swap: fix (unreached) data corruption (random-zebra)
0e71400 prevector: destroy elements only via erase() (random-zebra)
9811a68 [Core] Prevector type (random-zebra)

Pull request description:

  Based on:
  - [x] #1554

  This introduces `prevector<N, T>`, a new basic data type which is a fully API compatible drop-in replacement for `std::vector<T>` and uses it for the script, to reduce the considerable memory overhead of vectors in cases where they normally contain a small number of small elements.

  Original tests in Bitcoin showed a reduction in dbcache memory usage by **23%**, and made an initial sync **13%** faster.

  Backported from the following upstream's PRs, with only additional edits in the 2nd layer scripts (`masternode-payments`, `masternode-budget`) and in the unit tests (to address the differences in `insecure_rand`):

  - bitcoin#6914
  - bitcoin#7888
  - bitcoin#8850
  - bitcoin#9349
  - bitcoin#9505
  - bitcoin#9856
  - bitcoin#10534
  - bitcoin#11011
  - bitcoin#14028

  ~~NOTE: Updates to `memusage.h` needed as well, when #1531 is merged.~~

ACKs for top commit:
  furszy:
    Tested ACK a8b965e .
  Fuzzbawls:
    ACK a8b965e

Tree-SHA512: 203b660dd8d9f98780be660dae0ac951766ba438836bd6f0ec921e7749c1a84143f3b920fe49f2cc399f4aa7e763098f78746d3b6ff845bcf913bb13de984bc1

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14028: Explicitly initialize prevector _union

8dc8d9c

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14266: refactor: Lift prevector default vals to the mem…

3325df2

…ber declaration

d2eee87 Lift prevector default vals to the member declaration (Ben Woosley)

Pull request description:

  I overlooked this possibility in bitcoin#14028

ACKs for commit d2eee8:
  promag:
    utACK d2eee87, change looks good because members are always initialized.
  251Labs:
    utACK d2eee87 nice one.
  ken2812221:
    utACK d2eee87
  practicalswift:
    utACK d2eee87
  scravy:
    utACK d2eee87

Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14028: Explicitly initialize prevector _union

afe9198

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14266: refactor: Lift prevector default vals to the mem…

cdbf795

…ber declaration

d2eee87 Lift prevector default vals to the member declaration (Ben Woosley)

Pull request description:

  I overlooked this possibility in bitcoin#14028

ACKs for commit d2eee8:
  promag:
    utACK d2eee87, change looks good because members are always initialized.
  251Labs:
    utACK d2eee87 nice one.
  ken2812221:
    utACK d2eee87
  practicalswift:
    utACK d2eee87
  scravy:
    utACK d2eee87

Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14028: Explicitly initialize prevector _union

b480328

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14266: refactor: Lift prevector default vals to the mem…

612b7e8

…ber declaration

d2eee87 Lift prevector default vals to the member declaration (Ben Woosley)

Pull request description:

  I overlooked this possibility in bitcoin#14028

ACKs for commit d2eee8:
  promag:
    utACK d2eee87, change looks good because members are always initialized.
  251Labs:
    utACK d2eee87 nice one.
  ken2812221:
    utACK d2eee87
  practicalswift:
    utACK d2eee87
  scravy:
    utACK d2eee87

Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14028: Explicitly initialize prevector _union

431ef8f

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14028: Explicitly initialize prevector _union

aca3eb1

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14028: Explicitly initialize prevector _union

c62f8f6

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14028: Explicitly initialize prevector _union

493e0e8

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request


          Merge bitcoin#14028: Explicitly initialize prevector _union

1cbd426

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

# Conflicts:
#	src/prevector.h

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14028: Explicitly initialize prevector _union

2d91eb7

1d9aa00 Explicitly initialize prevector _union (Ben Woosley)

Pull request description:

Tree-SHA512: 3037a5d63b840a4cb0c3c26593ce1b7e1a6ba273a4ee5072563b20169be9783dbdfe3a38c9651d73b2d18ed9668deaf65f994eca7f225c70f875716f05eda3a6

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14266: refactor: Lift prevector default vals to the mem…

90f2117

…ber declaration

d2eee87 Lift prevector default vals to the member declaration (Ben Woosley)

Pull request description:

  I overlooked this possibility in bitcoin#14028

ACKs for commit d2eee8:
  promag:
    utACK d2eee87, change looks good because members are always initialized.
  251Labs:
    utACK d2eee87 nice one.
  ken2812221:
    utACK d2eee87
  practicalswift:
    utACK d2eee87
  scravy:
    utACK d2eee87

Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14266: refactor: Lift prevector default vals to the mem…

7b1d8c5

…ber declaration

d2eee87 Lift prevector default vals to the member declaration (Ben Woosley)

Pull request description:

  I overlooked this possibility in bitcoin#14028

ACKs for commit d2eee8:
  promag:
    utACK d2eee87, change looks good because members are always initialized.
  251Labs:
    utACK d2eee87 nice one.
  ken2812221:
    utACK d2eee87
  practicalswift:
    utACK d2eee87
  scravy:
    utACK d2eee87

Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14266: refactor: Lift prevector default vals to the mem…

bb27dde

…ber declaration

d2eee87 Lift prevector default vals to the member declaration (Ben Woosley)

Pull request description:

  I overlooked this possibility in bitcoin#14028

ACKs for commit d2eee8:
  promag:
    utACK d2eee87, change looks good because members are always initialized.
  251Labs:
    utACK d2eee87 nice one.
  ken2812221:
    utACK d2eee87
  practicalswift:
    utACK d2eee87
  scravy:
    utACK d2eee87

Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request


          Merge bitcoin#14266: refactor: Lift prevector default vals to the mem…

698ca44

…ber declaration

d2eee87 Lift prevector default vals to the member declaration (Ben Woosley)

Pull request description:

  I overlooked this possibility in bitcoin#14028

ACKs for commit d2eee8:
  promag:
    utACK d2eee87, change looks good because members are always initialized.
  251Labs:
    utACK d2eee87 nice one.
  ken2812221:
    utACK d2eee87
  practicalswift:
    utACK d2eee87
  scravy:
    utACK d2eee87

Tree-SHA512: f2726bae1cf892fd680cf8571027bcdc2e42ba567eaa901fb5fb5423b4d11b29e745e0163d82cb513d8c81399cc85933a16ed66d4a30829382d4721ffc41dc97

bitcoin locked as resolved and limited conversation to collaborators

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

Labels