Better c++ #1965
Better c++ #1965
Conversation
christopherlam
force-pushed
the
better-c++
branch
6 times, most recently
from
447f6b1
to
1e37258
Compare
christopherlam
force-pushed
the
better-c++
branch
2 times, most recently
from
27f6336
to
95cfcf9
Compare
That would be better as multiple PRs. It's ok to make a chain of them, just say in the comments who depends on who. |
christopherlam
force-pushed
the
better-c++
branch
from
4420619
to
d622929
Compare
christopherlam
force-pushed
the
better-c++
branch
3 times, most recently
from
c26698e
to
abb7efb
Compare
christopherlam
force-pushed
the
better-c++
branch
from
abb7efb
to
29b6034
Compare
| else | ||
| return false; | ||
| auto limit = *priv_limit; | ||
| if (limit) *balance = *limit; |
There was a problem hiding this comment.
That relies on an implicit cast from gnc_numeric to GncNumeric. Better to say *balance = gnc_numeric_create(limit->num, limit->denom);
| } | ||
| else | ||
| else if (auto priv_limit = get_kvp_gnc_numeric_path (acc, { KEY_BALANCE_LIMIT, key })) |
There was a problem hiding this comment.
Please keep KVP implementation details in QofInstance.
| @@ -113,8 +113,10 @@ typedef struct AccountPrivate | |||
| gnc_numeric cleared_balance; | |||
| gnc_numeric reconciled_balance; | |||
|
|
|||
| std::optional<gnc_numeric> higher_balance_limit; | |||
| std::optional<gnc_numeric> lower_balance_limit; | |||
| // outer optional to denote if balance_limit is cached | |||
There was a problem hiding this comment.
Too clever by half. Change your design so that the limits are loaded from KVP at object initialization and save the value to KVP in xaccAccountCommitEdit.
There was a problem hiding this comment.
Too clever by half. Change your design so that the limits are loaded from KVP at object initialization and save the value to KVP in xaccAccountCommitEdit.
Not too sure about that. gnc_account_init shouldn't assume a book is available to read kvp from. I'm thinking a 'balance_limits_are_cached' bool may be used. Or, forego caching them altogether.
There was a problem hiding this comment.
What? You can't create any other kind of QofInstance without a book, not that that has anything to do with this.
You have the wrong mental model of KVP. KVP is a storage extension to provide class member variables that don't require a matching XML element name and therefore require adding handler code to the backend. Ideally KVP wouldn't exist and we'd maintain and version datafiles, but we don't. Next best would have the KVP code in the backends and map directly to instance member variables so the engine wouldn't have to worry about them.
separate kvp getter/setter and the balance_limit caching logic
christopherlam
force-pushed
the
better-c++
branch
from
29b6034
to
8d8f53a
Compare
to be selectively merged after 5.7.
gnc_account_foreach_descendantfor c++ use.