Details

Reviewers

deadalnix
CCulianu
zquestz

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rSTAGINGfdd499448849: Add Amount class and use it for CENT and COIN.
rABCfdd499448849: Add Amount class and use it for CENT and COIN.

Test Plan

make VERBOSE=1 check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

shammah/camountnew

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 856
Build 856: arc lint + arc unit

Event Timeline

schancel created this revision.Sep 6 2017, 07:10

Owners added a reviewer: Restricted Owners Package.Sep 6 2017, 07:10

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 6 2017, 07:10

There should be a unit test for the CAmountNew class.

This revision now requires changes to proceed.Sep 6 2017, 10:10

Added:

Tests
Several integral-specific overrides
Integral-specific implicit constructor

Updating D529: Summary:

Add CAmountNewClass and use it for CENT and COIN.

Harbormaster completed remote builds in B855: Diff 1361.Sep 10 2017, 01:40

schancel retitled this revision from Summary: Add CAmountNewClass and use it for CENT and COIN. to Summary:Add CAmountNew class and use it for CENT and COIN..Sep 10 2017, 01:41

Added some comments and a question..

src/amount.h
32 ↗	(On Diff #1361)	Suggest: pass by reference eg const CAmountNew & in all of these.
36 ↗	(On Diff #1361)	Ditto... and ditto for all the operators below.
79 ↗	(On Diff #1361)	Question: Why are all of these friends and not just member functions? Any reason?

CCulianu added inline comments.Sep 11 2017, 05:54

src/amount.h
12 ↗	(On Diff #1361)	You might also want to #include <type_traits> here for header paranoia. std::enable_if lives in <type_traits> and as it happens now it got pulled in for us -- but this is not guaranteed into the infinite future of C++ impl's.. so I would suggest explicitly including the <type_traits> header.

CCulianu added inline comments.Sep 11 2017, 06:27

src/amount.h
43 ↗	(On Diff #1361)	Suggest: Implementing the real comparison only for operator< and operator== and then making operator >,<=, >=, != all be implemented in terms of those 2 basic operators. Eg: operator<=(lhs, rhs) { return !(lhs > rhs); } operator>=(lhs, rhs) { return !(lhs < rhs); } operator>(lhs, rhs) { return rhs < lhs; } operator!=(lhs, rhs) { return !(lhs == rhs); }
79 ↗	(On Diff #1361)	Nevermind: I see you want to take advantage of implicit conversion etc so yeah you need friend for these.

deadalnix added inline comments.Sep 11 2017, 18:02

src/amount.h
32 ↗	(On Diff #1361)	I don't think it is really necessary. This clearly work as a value type better than as a reference type. Now, because C++ doesn't differentiate between the 2 but if you get it wrong you'll regret it dearly (see slicing for instance) it's probably a good idea to come up with some naming convention that distinguish the 2. I suggest ISomething for interfaces, CSomething for reference type kind of classes and no prefix for value types. We assume value type are cheap to copy and if they aren't, make them non copiable by default. Does that work ? If so, then this class should just be named `Amount`

schancel planned changes to this revision.Sep 12 2017, 01:15

schancel marked 3 inline comments as done.

schancel added inline comments.

src/amount.h
12 ↗	(On Diff #1361)	Will do.
32 ↗	(On Diff #1361)	I'll make that change.
43 ↗	(On Diff #1361)	SGTM. Will do

Change operator implemntation to depend on < and == only.
Add include for type_traits.
Rename CAmountNew to Amount.

I'm not sure the division thing is what we want to do, but the overall thing looks solid.

src/amount.h
21	Maybe that's not necessary. I'd rather have a compile time error than a value assigned to 0 when I forget to initialize it.
24	I'm not sure why you need an enable_if here ? That seems a bit overkill. Why not just take a int64_t ? Other integral types will convert anyway. You also may want to declare this as explicit.
25	You can take by value so it'll match rvalues as well.
75	This will truncate and return a non monetary value. It doesn't looks correct to me.
81	Same as for `/`
src/validation.cpp
3808	Looks like you are using clang-format 4.0+ . Please use 3.8 for now.
src/wallet/test/wallet_tests.cpp
89	Put the comment on the preceding line.
194	dito
204	dito

schancel planned changes to this revision.Sep 13 2017, 02:38

schancel added inline comments.

src/amount.h
24	Double was implicitly converting. I wanted to ensure that didn't happen.
25	Tried it: ./amount.h:24:25: error: copy constructor must pass its first argument by reference Amount(const Amount _camount) : amount(_camount.amount) {} ^ const &
75	This is intentional. I do not want it to return a monetary type. Division and Modulus are not defined on money. If they're occurring you shouldn't get a monetary type back. I can remove them altogether though and just use .GetSatoshis() where needed.

Remove default constructor for Amount.
Remove modulus and division operators.

schancel retitled this revision from Summary:Add CAmountNew class and use it for CENT and COIN. to Add Amount class and use it for CENT and COIN..Sep 13 2017, 06:13

Remove commented out code accidently included in previous diff.

Harbormaster completed remote builds in B862: Diff 1370.Sep 13 2017, 14:57

deadalnix added inline comments.Sep 13 2017, 16:53

src/amount.h
21	As already said, i don't think this is necessary. Just taking an int64_t is enough as smaller integral convert to it.
75	3€ divided by 3 is 1€ . 3€ divided by 3€ is 1 (no unit).

schancel planned changes to this revision.Sep 14 2017, 05:58

schancel marked 13 inline comments as done.

schancel added inline comments.

src/amount.h
21	Tried the other suggestion, see: https://stackoverflow.com/questions/38772637/overload-ambiguous-int-int64-t-vs-int-double
75	Do you want these operations defined?

Move comments on wallet_tests.cpp to previous lines.

Harbormaster completed remote builds in B864: Diff 1372.Sep 14 2017, 05:59

schancel marked 6 inline comments as done.Sep 14 2017, 16:10

Change template constructor to a couple specializations

Harbormaster completed remote builds in B866: Diff 1376.Sep 15 2017, 06:04

deadalnix accepted this revision.Sep 15 2017, 23:24

This revision is now accepted and ready to land.Sep 15 2017, 23:24

Closed by commit rABCfdd499448849: Add Amount class and use it for CENT and COIN. (authored by schancel, committed by schancel). · Explain WhySep 16 2017, 01:19

This revision was automatically updated to reflect the committed changes.

freetrader mentioned this in rABC688e2640b992: Heal broken build (D529) by adding missing GetSatoshis() conversion calls.Sep 17 2017, 06:13

Add Amount class and use it for CENT and COIN.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Updating D529: Summary:

Revision Contents
Changeset List

Diff 1364

src/amount.h

src/amount.cpp

src/bench/ccoins_caching.cpp

src/bench/coin_selection.cpp

src/bench/mempool_eviction.cpp

src/chainparams.cpp

src/policy/fees.h

src/primitives/transaction.cpp

src/qt/bitcoinunits.cpp

src/qt/paymentserver.cpp

src/rpc/server.cpp

src/test/DoS_tests.cpp

src/test/amount_tests.cpp

src/test/compress_tests.cpp

src/test/main_tests.cpp

src/test/mempool_tests.cpp

src/test/miner_tests.cpp

src/test/rpc_tests.cpp

src/test/script_P2SH_tests.cpp

src/test/transaction_tests.cpp

src/test/txvalidationcache_tests.cpp

src/test/util_tests.cpp

src/txmempool.h

src/utilmoneystr.cpp

src/validation.h

src/validation.cpp

src/wallet/test/wallet_tests.cpp

src/wallet/wallet.h

Add Amount class and use it for CENT and COIN.ClosedPublicActions

Details

Diff Detail

Event Timeline

Updating D529: Summary:

Revision ContentsChangeset List

Diff 1364

src/amount.h

src/amount.cpp

src/bench/ccoins_caching.cpp

src/bench/coin_selection.cpp

src/bench/mempool_eviction.cpp

src/chainparams.cpp

src/policy/fees.h

src/primitives/transaction.cpp

src/qt/bitcoinunits.cpp

src/qt/paymentserver.cpp

src/rpc/server.cpp

src/test/DoS_tests.cpp

src/test/amount_tests.cpp

src/test/compress_tests.cpp

src/test/main_tests.cpp

src/test/mempool_tests.cpp

src/test/miner_tests.cpp

src/test/rpc_tests.cpp

src/test/script_P2SH_tests.cpp

src/test/transaction_tests.cpp

src/test/txvalidationcache_tests.cpp

src/test/util_tests.cpp

src/txmempool.h

src/utilmoneystr.cpp

src/validation.h

src/validation.cpp

src/wallet/test/wallet_tests.cpp

src/wallet/wallet.h

Add Amount class and use it for CENT and COIN.
ClosedPublic
Actions

Revision Contents
Changeset List