Skip to content

Conversation

@theStack
Copy link
Contributor

The functions ReadOrderPos and WriteOrderPos have been introduced in commit 9c7722b in 2012. Since accounts have been removed in #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").

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[].
@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #20452 (util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift)

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.

Copy link
Member

@jonatack jonatack left a 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.
@theStack theStack force-pushed the 202109-refactor-wallet-inline_readwrite_orderpos branch from f4adf0a to 98cf19c Compare September 12, 2021 21:52
@laanwj
Copy link
Member

laanwj commented Sep 16, 2021

This is a nice little cleanup.
Code review ACK 98cf19c

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;
Copy link
Member

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

@achow101
Copy link
Member

Code Review ACK 98cf19c

@fanquake fanquake merged commit f7189c4 into bitcoin:master Sep 17, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2021
…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
@theStack theStack deleted the 202109-refactor-wallet-inline_readwrite_orderpos branch September 21, 2021 13:37
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants