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 Passed
Unit
No Test Coverage
Build Status
Buildable 857
Build 857: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Sep 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 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

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?

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.

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.

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 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 ↗(On Diff #1364)

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 ↗(On Diff #1364)

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 ↗(On Diff #1364)

You can take by value so it'll match rvalues as well.

75 ↗(On Diff #1364)

This will truncate and return a non monetary value. It doesn't looks correct to me.

81 ↗(On Diff #1364)

Same as for /

src/validation.cpp
3808 ↗(On Diff #1364)

Looks like you are using clang-format 4.0+ . Please use 3.8 for now.

src/wallet/test/wallet_tests.cpp
89 ↗(On Diff #1364)

Put the comment on the preceding line.

194 ↗(On Diff #1364)

dito

204 ↗(On Diff #1364)

dito

schancel added inline comments.
src/amount.h
24 ↗(On Diff #1364)

Double was implicitly converting. I wanted to ensure that didn't happen.

25 ↗(On Diff #1364)

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 ↗(On Diff #1364)

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.
src/amount.h
21 ↗(On Diff #1364)

As already said, i don't think this is necessary. Just taking an int64_t is enough as smaller integral convert to it.

75 ↗(On Diff #1364)

3€ divided by 3 is 1€ . 3€ divided by 3€ is 1 (no unit).

schancel marked 13 inline comments as done.
schancel added inline comments.
src/amount.h
21 ↗(On Diff #1364)
75 ↗(On Diff #1364)

Do you want these operations defined?

schancel marked an inline comment as done.
  • Move comments on wallet_tests.cpp to previous lines.
  • Change template constructor to a couple specializations
This revision is now accepted and ready to land.Sep 15 2017, 23:24
This revision was automatically updated to reflect the committed changes.