Page MenuHomePhabricator

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

Authored by schancel on Sep 6 2017, 07:10.

Details

Test Plan

make VERBOSE=1 check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
shammah/camountnew
Lint
Lint OK
Unit
No Unit 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
deadalnix requested changes to this revision.Sep 6 2017, 10:10

There should be a unit test for the CAmountNew class.

This revision now requires changes to proceed.Sep 6 2017, 10:10
schancel updated this revision to Diff 1361.Sep 10 2017, 01:40
schancel edited edge metadata.

Added:

  • Tests
  • Several integral-specific overrides
  • Integral-specific implicit constructor

Updating D529: Summary:

Add CAmountNewClass and use it for CENT and COIN.

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
CCulianu edited edge metadata.Sep 10 2017, 19:05

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

schancel updated this revision to Diff 1364.Sep 12 2017, 02:16
  • 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.

schancel updated this revision to Diff 1365.Sep 13 2017, 04:35
  • 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
schancel updated this revision to Diff 1370.Sep 13 2017, 14:57
  • Remove commented out code accidently included in previous diff.
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
75

Do you want these operations defined?

schancel updated this revision to Diff 1372.Sep 14 2017, 05:59
schancel marked an inline comment as done.
  • Move comments on wallet_tests.cpp to previous lines.
schancel marked 6 inline comments as done.Sep 14 2017, 16:10
schancel updated this revision to Diff 1376.Sep 15 2017, 06:04
  • Change template constructor to a couple specializations
deadalnix accepted this revision.Sep 15 2017, 23:24
This revision is now accepted and ready to land.Sep 15 2017, 23:24
This revision was automatically updated to reflect the committed changes.