Skip to content
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

Small refactor of CCoinsViewCache::BatchWrite() #11353

Merged
merged 1 commit into from Nov 10, 2017
Merged

Small refactor of CCoinsViewCache::BatchWrite() #11353

merged 1 commit into from Nov 10, 2017

Conversation

danra
Copy link
Contributor

@danra danra commented Sep 16, 2017

std::unordered_map::erase( const_iterator pos ) returns an iterator to the element following the removed one. Use that to optimize (probably minor-performance-wise, and definitely code-structure-wise) the implementation of CCoinsViewCache::BatchWrite().

@danra
Small refactor of CCoinsViewCache::BatchWrite()
5b9748f 
std::unordered_map::erase( const_iterator pos ) returns an iterator to the element following the removed one. Use that to optimize (probably minor-performance-wise, and definitely code-structure-wise) the implementation of CCoinsViewCache::BatchWrite().
@meshcollider
Copy link
Contributor

Note to reviewers, most of the diff is indentation change, so use ?w=1

Other than adding missing braces to some if-statements which is a style only change, basically all this change does is move the erasure from mapIndex from the end of the loop body into the brackets.

@danra small changes like this aren't really that useful to be perfectly honest sorry, its time consuming to review things like this with almost no real benefit to it. It would be a lot more helpful if you wanted to help out in reviewing other, bigger PRs which are blockers for releases, since you obviously know the standards fairly well :)

@sipa
Copy link
Member

sipa commented Sep 17, 2017

@meshcollider Thanks, adding ?w=1 makes it look like a much more trivial refactor.

utACK 5b9748f

gmaxwell
gmaxwell approved these changes Sep 18, 2017
View reviewed changes
Copy link
Contributor

@gmaxwell gmaxwell left a comment

Choose a reason for hiding this comment

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

ACK.

@promag
Copy link
Member

promag commented Sep 18, 2017

IMO the only good change here is it = mapCoins.erase(it). Adding braces because indentation, and indentation because of an early continue doesn't sound worth it. Don't get me wrong, but there are lots of other possible refactors like these and I think we don't want that to happen.

That said, NACK as it is.

@gmaxwell
Copy link
Contributor

@promag If it weren't for the move of the erase into the afterthought I would have also been with you. Since it was being changed I think fixing up the indentation is OKAY.

@danra
Copy link
Contributor Author

danra commented Sep 18, 2017

@meshcollider

It would be a lot more helpful if you wanted to help out in reviewing other, bigger PRs which are blockers for releases, since you obviously know the standards fairly well :)

I will/am. I'm posting smaller fixes as I'm learning the code.

its time consuming to review things like this with almost no real benefit to it.

Improving both the conciseness and the structure of the code, making it more accessible and easy to reason about, does have real benefit IMHO.

@promag
The main change isn't the indentation, it's the more concise erase operation done in the for loop. Reducing indentation was a bonus.

@TheBlueMatt
Copy link
Contributor

utACK 5b9748f

@maflcko
Copy link
Member

maflcko commented Nov 10, 2017

utACK 5b9748f

@maflcko maflcko merged commit 5b9748f into bitcoin:master Nov 10, 2017
maflcko pushed a commit that referenced this pull request Nov 10, 2017
Merge #11353: Small refactor of CCoinsViewCache::BatchWrite()
05a7619 
5b9748f Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv)

Pull request description:

  `std::unordered_map::erase( const_iterator pos )` returns an iterator to the element following the removed one. Use that to optimize (probably minor-performance-wise, and definitely code-structure-wise) the implementation of `CCoinsViewCache::BatchWrite()`.

Tree-SHA512: 00abc838ad91771cfcddd45688841c9414869b75289d09b483a7f0ba835614fe189e9c8aca8a80e3de78ee397ec14083ae52e2e92b7863b3b6eb0d0cb892c9dd
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 30, 2019
Merge #11353: Small refactor of CCoinsViewCache::BatchWrite()
6ffbbc1 
Summary:
5b9748f Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv)

Pull request description:

  `std::unordered_map::erase( const_iterator pos )` returns an iterator to the element following the removed one. Use that to optimize (probably minor-performance-wise, and definitely code-structure-wise) the implementation of `CCoinsViewCache::BatchWrite()`.

Tree-SHA512: 00abc838ad91771cfcddd45688841c9414869b75289d09b483a7f0ba835614fe189e9c8aca8a80e3de78ee397ec14083ae52e2e92b7863b3b6eb0d0cb892c9dd

Backport of Core PR11353
bitcoin/bitcoin#11353

Review note: mostly indentation changes from the linter.  The main
changes are to the loop initialization, the first `if` statement, and
some bracket additions.

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3879
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 17, 2020
@PastaPastaPasta
Merge bitcoin#11353: Small refactor of CCoinsViewCache::BatchWrite()
adc66e7 
5b9748f Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv)

Pull request description:

  `std::unordered_map::erase( const_iterator pos )` returns an iterator to the element following the removed one. Use that to optimize (probably minor-performance-wise, and definitely code-structure-wise) the implementation of `CCoinsViewCache::BatchWrite()`.

Tree-SHA512: 00abc838ad91771cfcddd45688841c9414869b75289d09b483a7f0ba835614fe189e9c8aca8a80e3de78ee397ec14083ae52e2e92b7863b3b6eb0d0cb892c9dd
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
@PastaPastaPasta
Merge bitcoin#11353: Small refactor of CCoinsViewCache::BatchWrite()
a7b9dff 
5b9748f Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv)

Pull request description:

  `std::unordered_map::erase( const_iterator pos )` returns an iterator to the element following the removed one. Use that to optimize (probably minor-performance-wise, and definitely code-structure-wise) the implementation of `CCoinsViewCache::BatchWrite()`.

Tree-SHA512: 00abc838ad91771cfcddd45688841c9414869b75289d09b483a7f0ba835614fe189e9c8aca8a80e3de78ee397ec14083ae52e2e92b7863b3b6eb0d0cb892c9dd
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
@PastaPastaPasta
Merge bitcoin#11353: Small refactor of CCoinsViewCache::BatchWrite()
a969c8d 
5b9748f Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv)

Pull request description:

  `std::unordered_map::erase( const_iterator pos )` returns an iterator to the element following the removed one. Use that to optimize (probably minor-performance-wise, and definitely code-structure-wise) the implementation of `CCoinsViewCache::BatchWrite()`.

Tree-SHA512: 00abc838ad91771cfcddd45688841c9414869b75289d09b483a7f0ba835614fe189e9c8aca8a80e3de78ee397ec14083ae52e2e92b7863b3b6eb0d0cb892c9dd
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
@PastaPastaPasta
Merge bitcoin#11353: Small refactor of CCoinsViewCache::BatchWrite()
77933a3 
5b9748f Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv)

Pull request description:

  `std::unordered_map::erase( const_iterator pos )` returns an iterator to the element following the removed one. Use that to optimize (probably minor-performance-wise, and definitely code-structure-wise) the implementation of `CCoinsViewCache::BatchWrite()`.

Tree-SHA512: 00abc838ad91771cfcddd45688841c9414869b75289d09b483a7f0ba835614fe189e9c8aca8a80e3de78ee397ec14083ae52e2e92b7863b3b6eb0d0cb892c9dd
@random-zebra random-zebra mentioned this pull request Jun 29, 2021
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Jul 26, 2021
@furszy
Merge #2462: [Refactoring] Bitcoin 0.16 cherries
60d3629 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dcousens dcousens dcousens approved these changes

@gmaxwell gmaxwell gmaxwell approved these changes

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@danra @meshcollider @sipa @promag @gmaxwell @TheBlueMatt @maflcko @dcousens @fanquake