Refactor: clean up PeriodicFlush()#19085
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ⒶⒸⓀ |
|
Thanks @fanquake . I've rebased on master. |
jnewbery
force-pushed
the
2020-05-refactor-periodic-flush
branch
from
2495f8f to
9c03104
Compare
fanquake
left a comment
fanquake
left a comment
There was a problem hiding this comment.
ACK 9c031049be0a9b2fc9d8459adccdb56f28169ac0 - Thanks for the quick follow up. This is a nice simplification, and it'd seem that functions like BerkeleyDatabase::Backup or BerkeleyEnvironment::Flush could receive similar refactors.
There was one behaviour I noticed while reviewing (unchanged by this PR), which I've opened an issue for in #19175.
|
Rebased |
jnewbery
force-pushed
the
2020-05-refactor-periodic-flush
branch
from
9c03104 to
7c10020
Compare
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
ACK 9c031049be0a9b2fc9d8459adccdb56f28169ac0 nice refactoring
src/wallet/bdb.cpp
Outdated
There was a problem hiding this comment.
nit, while retouching this code, ++i is preferred over i++
| for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); it++) { | |
| for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); ++it) { |
There was a problem hiding this comment.
It makes no difference to the compiled code since we're not using the return value of the incrementer. I'll change to ++it if I have to retouch this branch, but otherwise it's not worth invalidating ACKs.
There was a problem hiding this comment.
nit, extra space after begin. Better, use range based loop.
There was a problem hiding this comment.
Yes. Even better. Will change if I have to retouch.
src/wallet/bdb.cpp
Outdated
There was a problem hiding this comment.
Will change if I need to retouch
|
Note to reviewers: I suggest looking at the diff with space changes ignored ( |
|
rebased |
jnewbery
force-pushed
the
2020-05-refactor-periodic-flush
branch
from
7c10020 to
e846a2a
Compare
|
ACK e846a2a 🎁 Show signature and timestampSignature: Timestamp of file with hash |
|
cc @achow101 any objections to this? |
src/wallet/bdb.cpp
Outdated
There was a problem hiding this comment.
nit, extra space after begin. Better, use range based loop.
|
I think this is ready for merge. It's a simple refactor and has 3 ACKs now. |
|
Went ahead to merge this and postponed to fix the nits in a related pull that had less ACKs (#18923) |
Summary: ``` PeriodicFlush() is much more convoluted than it needs to be: it has triple nesting, local variables counting refs and return values, and increments the mapFileUseCount iterator unnecessarily. Removing all of that makes the function much easier to understand. ``` Backport of core [[bitcoin/bitcoin#19085 | PR19085]]. Depends on D8617. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8618
6 participants
PeriodicFlush()is much more convoluted than it needs to be: it has triple nesting, local variables counting refs and return values, and increments themapFileUseCountiterator unnecessarily. Removing all of that makes the function much easier to understand.