-
Notifications
You must be signed in to change notification settings - Fork 38.6k
wallet: refactor: inline functions {Read,Write}OrderPos
#22941
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.
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
wallet: refactor: inline functions {Read,Write}OrderPos
#22941
Conversation
Since accounts were removed in commit c9c32e6, this function is only called at one place and thus can be as well inlined. Also, avoid a duplicate lookup by using the find() method and dereference, instead of calling count() and operator[].
|
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. |
jonatack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK f4adf0a846c4c4f00ca6cc9d73934289d9cfb985
These appear to replace two operations having logarithmic complexity in the size of the container with one. I don't know if modern compilers optimize these down to one operation on their own, but it doesn't seem to hurt.
Since accounts were removed in commit c9c32e6, this function is only called at one place and thus can be as well inlined.
Also, use a named cast for converting the atoi64() result into an unsigned int type.
f4adf0a to
98cf19c
Compare
|
This is a nice little cleanup. |
| ReadOrderPos(nOrderPos, mapValue); | ||
| nTimeSmart = mapValue.count("timesmart") ? (unsigned int)atoi64(mapValue["timesmart"]) : 0; | ||
| const auto it_op = mapValue.find("n"); | ||
| nOrderPos = (it_op != mapValue.end()) ? atoi64(it_op->second) : -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we'll want to replace the use of atoi64's here, which is one of the last remaining uses in non-test code, with ParseInt64 (as suggested by the developer notes).
|
Code Review ACK 98cf19c |
…OrderPos` 98cf19c wallet: refactor: avoid duplicate lookup on `mapValue["timesmart"]` (Sebastian Falbesoner) 973d8ba wallet: refactor: inline function WriteOrderPos() (Sebastian Falbesoner) 65ed198 wallet: refactor: inline function ReadOrderPos() (Sebastian Falbesoner) Pull request description: The functions `ReadOrderPos` and `WriteOrderPos` have been introduced in commit 9c7722b in 2012. Since accounts have been removed in bitcoin#13825 (commit c9c32e6), they are only called at one place in `CWalletTx::{Serialize,Unserialize}` and thus can be directly inlined instead. Additionally, this PR aims to avoids duplicate lookups on the map `mapValue` (affects keys "n" and "timesmart"). ACKs for top commit: laanwj: Code review ACK 98cf19c achow101: Code Review ACK 98cf19c Tree-SHA512: 8af63c174c79e589bd713f04e8e40caba9f93ec2978c805427cac50d48049808a8c23ff5eea9ef589c9bd79fc66087f43ff5ab28e3cda51dd03f37c0164e2e4c
The functions
ReadOrderPosandWriteOrderPoshave been introduced in commit 9c7722b in 2012. Since accounts have been removed in #13825 (commit c9c32e6), they are only called at one place inCWalletTx::{Serialize,Unserialize}and thus can be directly inlined instead. Additionally, this PR aims to avoids duplicate lookups on the mapmapValue(affects keys "n" and "timesmart").