Changing &var[0] to var.data() #10793

meshcollider commented at 9:45 AM on July 11, 2017: contributor

This just continues the work of #9804

Modifies a lot of &vector[]'s to vector.data()'s across all the files including tests, just the stuff that 9804 missed

laanwj added the label Refactoring on Jul 11, 2017

promag commented at 11:03 PM on July 12, 2017: member

#9804 should cherry pick this one right?

meshcollider commented at 11:37 PM on July 12, 2017: contributor

#9804 has just been merged

in src/rpc/server.cpp:447 in be9ade9fcc outdated

 443 | @@ -444,7 +444,7 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
 444 |      const std::vector<UniValue>& values = in.params.getValues();
 445 |      std::unordered_map<std::string, const UniValue*> argsIn;
 446 |      for (size_t i=0; i<keys.size(); ++i) {
 447 | -        argsIn[keys[i]] = &values[i];
 448 | +        argsIn[keys[i]] = values.data() + i;

jtimon commented at 8:21 PM on July 13, 2017:

Is this really clearer?

laanwj commented at 10:37 PM on September 6, 2017:

Agree with @jtimon here. This PR is titled Changing &var[0] to var.data(), this seems to overshoot that scope. Addressing out of an vector is UB no matter what, so this doesn't fix any potential issues, and isn't more readable IMO either.

in src/test/bloom_tests.cpp:158 in be9ade9fcc outdated

 153 | @@ -154,8 +154,8 @@ BOOST_AUTO_TEST_CASE(bloom_match)
 154 |      COutPoint prevOutPoint(uint256S("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0);
 155 |      {
 156 |          std::vector<unsigned char> data(32 + sizeof(unsigned int));
 157 | -        memcpy(&data[0], prevOutPoint.hash.begin(), 32);
 158 | -        memcpy(&data[32], &prevOutPoint.n, sizeof(unsigned int));
 159 | +        memcpy(data.data(), prevOutPoint.hash.begin(), 32);
 160 | +        memcpy(data.data()+32, &prevOutPoint.n, sizeof(unsigned int));

jtimon commented at 8:22 PM on July 13, 2017:

same here

dcousens commented at 11:26 PM on July 13, 2017:

This is inline with #9804, and prevents U/B if data is empty

laanwj commented at 7:04 PM on September 6, 2017:

If data is empty, I'd say copying to data()+32 is U/B no matter what.

in src/test/skiplist_tests.cpp:23 in be9ade9fcc outdated

  19 | @@ -20,13 +20,13 @@ BOOST_AUTO_TEST_CASE(skiplist_test)
  20 |  
  21 |      for (int i=0; i<SKIPLIST_LENGTH; i++) {
  22 |          vIndex[i].nHeight = i;
  23 | -        vIndex[i].pprev = (i == 0) ? NULL : &vIndex[i - 1];
  24 | +        vIndex[i].pprev = (i == 0) ? NULL : (vIndex.data() + i - 1);

jtimon commented at 8:23 PM on July 13, 2017:

same here

in src/test/skiplist_tests.cpp:29 in be9ade9fcc outdated

  26 |      }
  27 |  
  28 |      for (int i=0; i<SKIPLIST_LENGTH; i++) {
  29 |          if (i > 0) {
  30 | -            BOOST_CHECK(vIndex[i].pskip == &vIndex[vIndex[i].pskip->nHeight]);
  31 | +            BOOST_CHECK(vIndex[i].pskip == vIndex.data() + vIndex[i].pskip->nHeight);

jtimon commented at 8:23 PM on July 13, 2017:

here you aren't even consistent

dcousens commented at 10:57 PM on July 13, 2017:

what lacks consistency?

jtimon commented at 10:59 PM on July 13, 2017:

One time vIndex is accessed by vIndex.data() + n and another with vIndex[m]

meshcollider commented at 11:03 PM on July 13, 2017:

That's because one is accessing the element not a pointer...

dcousens commented at 11:24 PM on July 13, 2017:

The .data() usage here however, is not really inline with #9804. #9804 replaced usage of &v[0] and &v[N] for specifying a slice/range. In the change here, you are specifically referring to the memory location of 1 element, which isn't an rvalue, probably unnecessary to use .data().

meshcollider commented at 11:32 PM on July 13, 2017:

Isn't it still preferable to access the memory location of an element this way?

dcousens commented at 12:39 AM on July 14, 2017:

@MeshCollider the advantage of .data() is that it is defined to return nullptr if .empty(), compared to &v[0] potentially de-referencing unitialized memory. In the above, we assume vIndex is not empty, as you are de-referencing vIndex[i], therefore, it is probably not as necessary. As far as the pattern goes, #9804 introduced .data() specifically where &v[0] was used to target the front of an array, which may have been uninitialized. That is not the case here.

in src/test/skiplist_tests.cpp:40 in be9ade9fcc outdated

  36 | @@ -37,9 +37,9 @@ BOOST_AUTO_TEST_CASE(skiplist_test)
  37 |          int from = InsecureRandRange(SKIPLIST_LENGTH - 1);
  38 |          int to = InsecureRandRange(from + 1);
  39 |  
  40 | -        BOOST_CHECK(vIndex[SKIPLIST_LENGTH - 1].GetAncestor(from) == &vIndex[from]);
  41 | -        BOOST_CHECK(vIndex[from].GetAncestor(to) == &vIndex[to]);
  42 | -        BOOST_CHECK(vIndex[from].GetAncestor(0) == &vIndex[0]);
  43 | +        BOOST_CHECK(vIndex[SKIPLIST_LENGTH - 1].GetAncestor(from) == vIndex.data() + from);

jtimon commented at 8:24 PM on July 13, 2017:

well, you get the idea, if the index was other than 0, it seems to me that the change makes things worse.

jtimon commented at 8:25 PM on July 13, 2017: contributor

Fast review ACK modulo nits.

meshcollider commented at 11:06 PM on July 13, 2017: contributor

@jtimon this is consistent with how #9804 did it

meshcollider commented at 10:01 AM on July 14, 2017: contributor

Reverted the changes where specific elements were accessed, thanks @jtimon and @dcousens

meshcollider commented at 8:58 AM on August 15, 2017: contributor

Rebased

meshcollider commented at 12:07 PM on August 28, 2017: contributor

Rebased

in src/qt/test/paymentservertests.cpp:46 in db0e991fb7 outdated

  42 | @@ -43,7 +43,7 @@ static SendCoinsRecipient handleRequest(PaymentServer* server, std::vector<unsig
  43 |      // Write data to a temp file:
  44 |      QTemporaryFile f;
  45 |      f.open();
  46 | -    f.write((const char*)&data[0], data.size());
  47 | +    f.write((const char*) data.data(), data.size());

laanwj commented at 10:25 PM on September 5, 2017:

nit: Please remove space after the case here

MarcoFalke commented at 10:33 PM on September 5, 2017:

@MeshCollider You can pipe the diff through clang-format to prevent such nits in the future. See dev notes.

meshcollider commented at 11:08 PM on September 5, 2017:

Fixed, thanks :)

laanwj commented at 10:29 PM on September 5, 2017: member

utACK [s]db0e991fb7975c071818b058376644be08ced343[/s] https://github.com/bitcoin/bitcoin/pull/10793/commits/7d5d19903168bb9452e386ace62b74875c04dae2

laanwj assigned laanwj on Sep 7, 2017

laanwj commented at 9:36 PM on September 7, 2017: member

The commit message is weird now :)

Changing &vec[0] to vec.data(), what 9804 missed 592404f03f

meshcollider commented at 10:37 PM on September 7, 2017: contributor

Argh fixed, thanks :)

laanwj merged this on Sep 7, 2017

laanwj closed this on Sep 7, 2017

laanwj referenced this in commit efb4383ef6 on Sep 7, 2017

meshcollider deleted the branch on Sep 14, 2017

schancel referenced this in commit d1cf65ab2e on Jul 18, 2019

PastaPastaPasta referenced this in commit 45025cec6d on Sep 23, 2019

PastaPastaPasta referenced this in commit a37a5009ee on Dec 21, 2019

PastaPastaPasta referenced this in commit 2b82bd4b70 on Jan 2, 2020

PastaPastaPasta referenced this in commit 82e93d37ff on Jan 4, 2020

PastaPastaPasta referenced this in commit c960f49af6 on Jan 4, 2020

PastaPastaPasta referenced this in commit 40c0fb579b on Jan 10, 2020

PastaPastaPasta referenced this in commit 0e136836b4 on Jan 10, 2020

PastaPastaPasta referenced this in commit 75ec11ee93 on Jan 10, 2020

PastaPastaPasta referenced this in commit 0ac78523bb on Jan 12, 2020

ckti referenced this in commit f91e3cdb8f on Mar 28, 2021

gades referenced this in commit b3b8e24d13 on Jun 28, 2021

furszy referenced this in commit 60d36292bc on Jul 26, 2021

DrahtBot locked this on Sep 8, 2021