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

meshcollider · 2017-07-11T09:45:31Z

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

promag · 2017-07-12T23:03:53Z

#9804 should cherry pick this one right?

meshcollider · 2017-07-12T23:37:43Z

#9804 has just been merged

jtimon · 2017-07-13T20:21:48Z

src/rpc/server.cpp

Is this really clearer?

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.

jtimon · 2017-07-13T20:22:30Z

src/test/bloom_tests.cpp

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

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

jtimon · 2017-07-13T20:23:04Z

src/test/skiplist_tests.cpp

jtimon · 2017-07-13T20:23:34Z

src/test/skiplist_tests.cpp

here you aren't even consistent

what lacks consistency?

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

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

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().

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

@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.

jtimon · 2017-07-13T20:24:39Z

src/test/skiplist_tests.cpp

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

jtimon · 2017-07-13T20:25:06Z

Fast review ACK modulo nits.

meshcollider · 2017-07-13T23:06:32Z

@jtimon this is consistent with how #9804 did it

meshcollider · 2017-07-14T10:01:22Z

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

meshcollider · 2017-08-15T08:58:01Z

Rebased

meshcollider · 2017-08-28T12:07:07Z

Rebased

laanwj · 2017-09-05T22:25:19Z

src/qt/test/paymentservertests.cpp

nit: Please remove space after the case here

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

Fixed, thanks :)

laanwj · 2017-09-05T22:29:15Z

utACK [s]db0e991fb7975c071818b058376644be08ced343[/s] 7d5d199

laanwj · 2017-09-07T21:36:13Z

The commit message is weird now :)

meshcollider · 2017-09-07T22:37:33Z

Argh fixed, thanks :)

592404f Changing &vec[0] to vec.data(), what 9804 missed (MeshCollider) Pull request description: 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 Tree-SHA512: dd1a9dffb999dea4fba78dcc91fe02f90250db86f5c74948e1ff3e8b4036b2154b600555eaa04dece5368920aae3513bc36425dc96e4319ca1041b0928a6b656

Summary: 592404f03 Changing &vec[0] to vec.data(), what 9804 missed (MeshCollider) Pull request description: This just continues the work of bitcoin/bitcoin#9804 Modifies a lot of `&vector[]`'s to `vector.data()`'s across all the files including tests, just the stuff that 9804 missed Tree-SHA512: dd1a9dffb999dea4fba78dcc91fe02f90250db86f5c74948e1ff3e8b4036b2154b600555eaa04dece5368920aae3513bc36425dc96e4319ca1041b0928a6b656 Backport of Core PR10793 bitcoin/bitcoin#10793 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3656

592404f Changing &vec[0] to vec.data(), what 9804 missed (MeshCollider) Pull request description: This just continues the work of bitcoin#9804 Modifies a lot of `&vector[]`'s to `vector.data()`'s across all the files including tests, just the stuff that 9804 missed Tree-SHA512: dd1a9dffb999dea4fba78dcc91fe02f90250db86f5c74948e1ff3e8b4036b2154b600555eaa04dece5368920aae3513bc36425dc96e4319ca1041b0928a6b656

7a5d181 Use the character based overload for std::string::find. (Alin Rus) c19401b Don't use pass by reference to const for cheaply-copied types (bool, char, etc.). (practicalswift) 4c5fe36 [Refactor] Remove unused fQuit var from checkqueue.h (donaloconnor) fda7a5f Cleanup: remove unneeded header includes (random-zebra) ac2476c Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512 (practicalswift) a346262 Fix code constness in CBlockIndex::GetAncestor() overloads (Dan Raviv) 65e3f4e Refactor: More range-based for loops (random-zebra) dd3d3c4 Use range-based for loops (C++11) when looping over map elements (practicalswift) 5a7750a Move RPC registration out of AppInitParameterInteraction (Russell Yanofsky) 402b4c4 Use compile-time constants instead of unnamed enumerations (practicalswift) bbac2d0 [Trivial] Fix indentation in coins.cpp (random-zebra) e539c62 Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv) ec91759 Use MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)) (random-zebra) 93487b1 Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree (random-zebra) ff43d69 Use unique_ptr for pdbCopy (Db) and fix potential memory leak (practicalswift) b4d9641 Use unique_ptr for dbenv (DbEnv) (practicalswift) 36108b9 Use unique_ptr for pfilter (CBloomFilter) (practicalswift) ff1c454 Use unique_ptr for sem{Addnode,Outbound} (CSemaphore) (practicalswift) 93daf17 Use unique_ptr for httpRPCTimerInterface (HTTPRPCTimerInterface) (practicalswift) 020ac16 Init: Remove redundant exit(EXIT_FAILURE) instances and replace with (random-zebra) b9f5d1f Remove duplicate uriParts.size() > 0 check (practicalswift) 440d961 Remove redundant check (!ecc is always true) (practicalswift) bfd295b Remove redundant NULL checks after new (practicalswift) 97aad32 Make fUseCrypto atomic (MeshCollider) 2711f78 Consistent parameter names in txdb.h (MeshCollider) d40df3a Fix race for mapBlockIndex in AppInitMain (random-zebra) 03b7766 Cleanup: remove unused functions to Hash the concat of 4 or more objects (random-zebra) c520e0f Remove some unused functions and methods (Pieter Wuille) 508f1a1 range-based loops and const qualifications in net.cpp (Marko Bencun) 79b1e50 Refactor: implement CPubKey::data() (random-zebra) 614d26c Refactor: more &vec[0] to vec.data() (random-zebra) 02b6337 Changing &vec[0] to vec.data(), what 9804 missed (MeshCollider) c1c8b05 Ensure that data types are consistent (jjz) 732bb9d Fix potential null dereferences (MeshCollider) 80f35f9 Remove unreachable code (practicalswift) Pull request description: This is a collection of simple refactorings coming from upstream Bitcoin v0.16 (adapting/extending to PIVX-specific code where needed). Pull requests backported: - bitcoin#10845 (practicalswift) - bitcoin#11238 (MeshCollider) - bitcoin#11232 (jjz) - bitcoin#10793 (MeshCollider) - bitcoin#10888 (benma) - ~~bitcoin#11351 (danra)~~ [edit: removed - included in #2423] - bitcoin#11385 (sipa) - bitcoin#11107 (MeshCollider) - bitcoin#10898 (practicalswift) - bitcoin#11511 (donaloconnor) - bitcoin#11043 (practicalswift) - bitcoin#11353 (danra) - bitcoin#10749 (practicalswift) - bitcoin#11603 (ryanofsky) - bitcoin#10493 (practicalswift) - bitcoin#11337 (danra) - bitcoin#11516 (practicalswift) - bitcoin#10574 (practicalswift) - bitcoin#12108 (donaloconnor) - bitcoin#10839 (practicalswift) - bitcoin#12159 (kekimusmaximus) ACKs for top commit: Fuzzbawls: ACK 7a5d181 furszy: re-ACK 7a5d181 after rebase, no code changes. Merging.. Tree-SHA512: d92f5df47f443391a6531274a2efb9a4882c62d32eff628f795b3abce703f108c8b40ec80ac841cde6c5fdd5c9ff2b6056a31546ac2edda279f5f18fccc99c32

laanwj added the Refactoring label Jul 11, 2017

jtimon reviewed Jul 13, 2017

View reviewed changes

src/test/skiplist_tests.cpp Outdated

Copy link

Contributor

jtimon Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

jtimon reviewed Jul 13, 2017

View reviewed changes

laanwj reviewed Sep 5, 2017

View reviewed changes

laanwj self-assigned this Sep 7, 2017

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

592404f

laanwj merged commit 592404f into bitcoin:master Sep 7, 2017

meshcollider deleted the prefer-vector-data branch September 14, 2017 05:59

random-zebra mentioned this pull request Jun 29, 2021

[Refactoring] Bitcoin 0.16 cherries PIVX-Project/PIVX#2462

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

meshcollider commented Jul 11, 2017

Uh oh!

promag commented Jul 12, 2017

Uh oh!

meshcollider commented Jul 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jul 13, 2017

Uh oh!

meshcollider commented Jul 13, 2017

Uh oh!

meshcollider commented Jul 14, 2017

Uh oh!

meshcollider commented Aug 15, 2017

Uh oh!

meshcollider commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 7, 2017

Uh oh!

meshcollider commented Sep 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dcousens Jul 13, 2017 •

edited

Loading

dcousens Jul 13, 2017 •

edited

Loading

dcousens Jul 14, 2017 •

edited

Loading

laanwj commented Sep 5, 2017 •

edited

Loading