wallet: refactor: inline functions `{Read,Write}OrderPos` #22941

pull theStack wants to merge 3 commits into bitcoin:master from theStack:202109-refactor-wallet-inline_readwrite_orderpos changing 1 files +7 −22

theStack commented at 2:44 PM on September 10, 2021: member

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

wallet: refactor: inline function ReadOrderPos()

Since accounts were removed in commit c9c32e6b844fc79467b7e24c6c916142a0d08484,
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[].

65ed198295

DrahtBot added the label Wallet on Sep 10, 2021
DrahtBot commented at 2:38 AM on September 11, 2021: member


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.
in src/wallet/transaction.h:177 in f4adf0a846 outdated
```
 172 | @@ -192,10 +173,8 @@ class CWalletTx
 173 |          mapValue_t mapValueCopy = mapValue;
 174 |  
 175 |          mapValueCopy["fromaccount"] = "";
 176 | -        WriteOrderPos(nOrderPos, mapValueCopy);
 177 | -        if (nTimeSmart) {
 178 | -            mapValueCopy["timesmart"] = strprintf("%u", nTimeSmart);
 179 | -        }
 180 | +        if (nOrderPos != -1) mapValueCopy["n"] = ToString(nOrderPos);
 181 | +        if (nTimeSmart) mapValueCopy["timesmart"] = strprintf("%u", nTimeSmart);
```
jonatack commented at 1:59 PM on September 12, 2021:

This nTimeSmart change (dropping the conditional brackets) isn't needed and is also maybe a bit harder to read, or use with gdb, as the debugger would stop on the same line for both the condition and the statement. For the same reason, I've been leaning more and more toward using brackets instead of one-line conditionals, like for the nOrderPos line too, except for very simple statements like just return in a guard clause.

theStack commented at 9:54 PM on September 12, 2021:

Good point considering the potential debugging issue on condition-and-statement-one-liners, never saw it from that perspective (probably because I rarely use debuggers)! Put the braces back and also add the nOrderPos part in the same style.

in src/wallet/transaction.h:217 in f4adf0a846 outdated

 210 | @@ -232,8 +211,10 @@ class CWalletTx
 211 |              setConfirmed();
 212 |          }
 213 |  
 214 | -        ReadOrderPos(nOrderPos, mapValue);
 215 | -        nTimeSmart = mapValue.count("timesmart") ? (unsigned int)atoi64(mapValue["timesmart"]) : 0;
 216 | +        const auto it_op = mapValue.find("n");
 217 | +        nOrderPos = (it_op != mapValue.end()) ? atoi64(it_op->second) : -1;
 218 | +        const auto it_ts = mapValue.find("timesmart");
 219 | +        nTimeSmart = (it_ts != mapValue.end()) ? (unsigned int)atoi64(it_ts->second) : 0;

jonatack commented at 2:02 PM on September 12, 2021:

Maybe use a named cast while touching this line instead of a C-style cast.

theStack commented at 9:54 PM on September 12, 2021:

Thanks, that seems reasonable, done.

jonatack commented at 2:11 PM on September 12, 2021: member

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.

wallet: refactor: inline function WriteOrderPos()

Since accounts were removed in commit c9c32e6b844fc79467b7e24c6c916142a0d08484,
this function is only called at one place and thus can be as well inlined.

973d8ba93d

wallet: refactor: avoid duplicate lookup on `mapValue["timesmart"]`

Also, use a named cast for converting the atoi64() result into an
unsigned int type.

98cf19ca32

theStack force-pushed on Sep 12, 2021
laanwj commented at 5:28 PM on September 16, 2021: member

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

in src/wallet/transaction.h:219 in 98cf19ca32

 214 | @@ -232,8 +215,10 @@ class CWalletTx
 215 |              setConfirmed();
 216 |          }
 217 |  
 218 | -        ReadOrderPos(nOrderPos, mapValue);
 219 | -        nTimeSmart = mapValue.count("timesmart") ? (unsigned int)atoi64(mapValue["timesmart"]) : 0;
 220 | +        const auto it_op = mapValue.find("n");
 221 | +        nOrderPos = (it_op != mapValue.end()) ? atoi64(it_op->second) : -1;

laanwj commented at 5:36 PM on September 16, 2021:

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 commented at 6:16 PM on September 16, 2021: member

Code Review ACK 98cf19ca32785c991628324c313e01349c2986af
fanquake merged this on Sep 17, 2021
fanquake closed this on Sep 17, 2021
sidhujag referenced this in commit 61c8a62496 on Sep 19, 2021
theStack deleted the branch on Sep 21, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

#13825 [wallet] Kill accounts

wallet: refactor: inline functions `{Read,Write}OrderPos` #22941

Conflicts