Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Changing &var[0] to var.data() #10793
Conversation
laanwj
added the
Refactoring
label
Jul 11, 2017
|
#9804 should cherry pick this one right? |
MeshCollider
commented
Jul 12, 2017
|
#9804 has just been merged |
| @@ -444,7 +444,7 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c | ||
| const std::vector<UniValue>& values = in.params.getValues(); | ||
| std::unordered_map<std::string, const UniValue*> argsIn; | ||
| for (size_t i=0; i<keys.size(); ++i) { | ||
| - argsIn[keys[i]] = &values[i]; | ||
| + argsIn[keys[i]] = values.data() + i; |
| - memcpy(&data[0], prevOutPoint.hash.begin(), 32); | ||
| - memcpy(&data[32], &prevOutPoint.n, sizeof(unsigned int)); | ||
| + memcpy(data.data(), prevOutPoint.hash.begin(), 32); | ||
| + memcpy(data.data()+32, &prevOutPoint.n, sizeof(unsigned int)); |
| @@ -20,13 +20,13 @@ BOOST_AUTO_TEST_CASE(skiplist_test) | ||
| for (int i=0; i<SKIPLIST_LENGTH; i++) { | ||
| vIndex[i].nHeight = i; | ||
| - vIndex[i].pprev = (i == 0) ? NULL : &vIndex[i - 1]; | ||
| + vIndex[i].pprev = (i == 0) ? NULL : (vIndex.data() + i - 1); |
| vIndex[i].BuildSkip(); | ||
| } | ||
| for (int i=0; i<SKIPLIST_LENGTH; i++) { | ||
| if (i > 0) { | ||
| - BOOST_CHECK(vIndex[i].pskip == &vIndex[vIndex[i].pskip->nHeight]); | ||
| + BOOST_CHECK(vIndex[i].pskip == vIndex.data() + vIndex[i].pskip->nHeight); |
jtimon
Jul 13, 2017
Member
One time vIndex is accessed by vIndex.data() + n and another with vIndex[m]
dcousens
Jul 13, 2017
•
Contributor
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
Jul 13, 2017
Isn't it still preferable to access the memory location of an element this way?
dcousens
Jul 14, 2017
•
Contributor
@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.
| - BOOST_CHECK(vIndex[SKIPLIST_LENGTH - 1].GetAncestor(from) == &vIndex[from]); | ||
| - BOOST_CHECK(vIndex[from].GetAncestor(to) == &vIndex[to]); | ||
| - BOOST_CHECK(vIndex[from].GetAncestor(0) == &vIndex[0]); | ||
| + BOOST_CHECK(vIndex[SKIPLIST_LENGTH - 1].GetAncestor(from) == vIndex.data() + from); |
jtimon
Jul 13, 2017
Member
well, you get the idea, if the index was other than 0, it seems to me that the change makes things worse.
|
Fast review ACK modulo nits. |
MeshCollider
commented
Jul 13, 2017
MeshCollider commentedJul 11, 2017
This just continues the work of #9804
Modifies a lot of
&vector[]'s tovector.data()'s across all the files including tests, just the stuff that 9804 missed