I strongly recommend multiplying by a scalar instead (i.e., just a plain int or int64).

Quantities are implicitly typed (either milliCPU or RAM) and multiplying them together gives units like squared milliCPU (which would be microCPU) or squared bytes. For your use case I think multiplying by a scalar makes much more sense. The implementation will be simpler too.

The actual invariant you need to test / prove is that repeated addition gives the same answer as the multiplication. The point of this type is that there isn't floating point error accumulation.

I think the signature of this function needs to be able to visibly fail on overflow.

or if not overflow, it could have a return value indicating when the result is inexact (e.g. because it had to change scale to avoid overflow and repeated addition might get a different final answer).

Thank you for some suggestions!

I strongly recommend multiplying by a scalar instead (i.e., just a plain int or int64).

It sounds much more reasonable to me.

I will change the implementation so that this function has an arg typed int64 since I don't have any reasons to want to use a Quantity instead of a scaler.

IIUC, I can address your other comments once I change this implementation as you suggested (Quantity -> scalar).

This isn't the thing to do at all (see my other comment), but for future reference, the natural way to multiply two numbers in the form a * b ^ c is just a1*a2 * b^(c1+c2). That is, values multiply and scales add. Great care needs to be taken when values are large.

Good explanation. Thanks, @lavalamp .

"... the natural way to multiply two numbers in the form a * b ^ c is just a1*a2 * b^(c1+c2). That is, values multiply and scales add. Great care needs to be taken when values are large.
"

These last three test cases are very careful to test the cases where the algorithm returns correct answers. Instead, they need to test the cases where the algorithm overflows. I would expect that some overflow can be prevented by changing scale.

I would expect that some overflow can be prevented by changing scale.

In other operators for an int64Amount, such as Add and Sub, they don't change a scale if overflow or underflow would result, and then they would return false.

So, I implemented Mul so that Mul returns false, not changing a scale.

Note: In Quantity, Mul avoids overflow by changing a scale.

In addition to my other reason for using a scalar -- I believe changing the format will destroy the property that multiplication needs to give the exact result of repeated addition. I don't think you should do this.

If the result is !ok, should we be relying on the result at all? What I mean is I think it maybe should look like:

if ok && !test.ok {
    t.Errorf("unexpected success: %v", c)
} else if !ok && test.ok {
    t.Errorf("unexpected failure")
} else if ok {
    // confirm result is correct
} else {
    // confirm source is not modified
}

Emphasis being that if ok != test.ok, we do not look any further. Then, in all the cases above where the result is expected to be false, just set c to int64Amount{} so it isn't distracting?

Sounds good to me. I will apply the suggestions!

Done.

I feel like I'm a pest, but these cases still feel sort of haphazard, can we be a little more systematic? E.g. something like:

https://go.dev/play/p/k4IOkuEvrXD

That may be a bit much, but I hope you see what I mean - I'm staring at these big tables of struct initisalisations, some with function calls in-between and trying to convince myself that all the edge case are covered. Like, 0 * 0 isn't covered. Probably should be.

I really appreciate your review :)
I'll try to make tests more systematic and add a test case for 0 * 0.

@thockin I tried to make test cases more systematic. However, because we need to define expected ok against test cases in advance, it was hard to make test cases more systematic using the way you suggested.

So, how about just adding a formula as a test name like this:

"100×10^1 × 1000 = 100000×10^1": {int64Amount{value: 100, scale: 1}, 1000, int64Amount{value: 100000, scale: 1}, true}
"1×10^100 × 10 = OVERFLOW": {int64Amount{value: 1, scale: 100}, 10, int64Amount{value: 1, scale: 100}, false},

Although this way couldn't make test cases systematic, I believe that this way makes it easier to find leaking edge cases.

I love the idea of adding useful names

Maybe also, for error cases, make t.c be int64Amount{} since we the test loop should not even look at it.

Maybe also, for error cases, make t.c be int64Amount{} since we the test loop should not even look at it.

SGTM