<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 29, 2015 at 4:52 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank" class="cremed">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><br>
> +#define SCALED_NUMBER_SHIFT(op, base)                                          \<br>
> +  template <class DigitsT>                                                     \<br>
> +  ScaledNumber<DigitsT> operator op(const ScaledNumber<DigitsT> &L,            \<br>
> +                                    int16_t Shift) {                           \<br>
> +    return ScaledNumber<DigitsT>(L) base Shift;                                \<br>
> +  }<br>
> +SCALED_NUMBER_SHIFT(<<, <<= )<br>
> +SCALED_NUMBER_SHIFT(>>, >>= )<br>
> +#undef SCALED_NUMBER_SHIFT<br>
<br>
</div></div>With only two in this group, do you think the macro is still adding<br>
value?  If not, feel free to rewrite without the macro.  Up to you.<br></blockquote><div><br></div><div>Good point. Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> -  // 64-bit division is hard to test, since divide64 doesn't canonicalized its<br>
> +  // 64-bit division is hard to test, since divide64 doesn't canonicalize its<br>
<br>
</span>Maybe commit this separately?  It seems unrelated.<br>
<span class=""><br></span></blockquote><div><br></div><div>Done.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>    // output.  However, this is the algorithm the implementation uses:<br>
>    //<br>
>    // - Shift divisor right.<br>
> @@ -532,4 +532,17 @@<br>
>    EXPECT_EQ(SP64(0, 0), getDifference64(1, -64, 1, -1));<br>
>  }<br>
><br>
> +TEST(ScaledNumberHelpersTest, arithmeticOperators) {<br>
<br>
</span>Can you duplicate all the EXPECTs here for `uint64_t` as well?<br>
<span class=""><br></span></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) + ScaledNumber<uint32_t>(1, 1),<br>
> +            ScaledNumber<uint32_t>(10, 0));<br>
<br>
</span>IIRC, the error messages for EXPECT look better if the expected value<br>
is the LHS of the macro.  It prints something like this when it fails:<br>
<br>
    Expected value: <LHS><br>
    Actual value:   <RHS><br>
<br>
So it would be better if you reversed the arguments for all of these.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) - ScaledNumber<uint32_t>(1, 1),<br>
> +            ScaledNumber<uint32_t>(6, 0));<br>
> +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) * ScaledNumber<uint32_t>(1, 1),<br>
> +            ScaledNumber<uint32_t>(2, 3));<br>
> +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) / ScaledNumber<uint32_t>(1, 1),<br>
> +            ScaledNumber<uint32_t>(1, 2));<br>
> +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) >> 1, ScaledNumber<uint32_t>(1, 2));<br>
> +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) << 1, ScaledNumber<uint32_t>(2, 3));<br>
<br>
</span>For clarity, I'd spell this last expected value as<br>
`ScaledNumber<uint32_t>(1, 4)`.  I think that makes it easier to<br>
compare with the previous line.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">For clarity, these last two didn't previously compile for `uint64_t`,<br>
did they?  (If they did, then I'm confused...)<br></blockquote><div><br></div><div>Right. This test case does not even compile without the patch.</div><div><br></div><div><br></div><div>Thanks. Diego.</div></div></div></div>