[PATCH] Fix private constructor for ScaledNumber.

[Diego Novillo]

On Wed, Apr 29, 2015 at 4:52 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > +#define SCALED_NUMBER_SHIFT(op, base)
>         \
> > +  template <class DigitsT>
>        \
> > +  ScaledNumber<DigitsT> operator op(const ScaledNumber<DigitsT> &L,
>         \
> > +                                    int16_t Shift) {
>        \
> > +    return ScaledNumber<DigitsT>(L) base Shift;
>         \
> > +  }
> > +SCALED_NUMBER_SHIFT(<<, <<= )
> > +SCALED_NUMBER_SHIFT(>>, >>= )
> > +#undef SCALED_NUMBER_SHIFT
>
> With only two in this group, do you think the macro is still adding
> value?  If not, feel free to rewrite without the macro.  Up to you.
>

Good point. Done.


>
> > -  // 64-bit division is hard to test, since divide64 doesn't
> canonicalized its
> > +  // 64-bit division is hard to test, since divide64 doesn't
> canonicalize its
>
> Maybe commit this separately?  It seems unrelated.
>
>
Done.



> >    // output.  However, this is the algorithm the implementation uses:
> >    //
> >    // - Shift divisor right.
> > @@ -532,4 +532,17 @@
> >    EXPECT_EQ(SP64(0, 0), getDifference64(1, -64, 1, -1));
> >  }
> >
> > +TEST(ScaledNumberHelpersTest, arithmeticOperators) {
>
> Can you duplicate all the EXPECTs here for `uint64_t` as well?
>
>
Done.


> > +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) + ScaledNumber<uint32_t>(1, 1),
> > +            ScaledNumber<uint32_t>(10, 0));
>
> IIRC, the error messages for EXPECT look better if the expected value
> is the LHS of the macro.  It prints something like this when it fails:
>
>     Expected value: <LHS>
>     Actual value:   <RHS>
>
> So it would be better if you reversed the arguments for all of these.
>

Done.


>
> > +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) - ScaledNumber<uint32_t>(1, 1),
> > +            ScaledNumber<uint32_t>(6, 0));
> > +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) * ScaledNumber<uint32_t>(1, 1),
> > +            ScaledNumber<uint32_t>(2, 3));
> > +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) / ScaledNumber<uint32_t>(1, 1),
> > +            ScaledNumber<uint32_t>(1, 2));
> > +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) >> 1,
> ScaledNumber<uint32_t>(1, 2));
> > +  EXPECT_EQ(ScaledNumber<uint32_t>(1, 3) << 1,
> ScaledNumber<uint32_t>(2, 3));
>
> For clarity, I'd spell this last expected value as
> `ScaledNumber<uint32_t>(1, 4)`.  I think that makes it easier to
> compare with the previous line.
>

Done.


> For clarity, these last two didn't previously compile for `uint64_t`,
> did they?  (If they did, then I'm confused...)
>

Right. This test case does not even compile without the patch.


Thanks. Diego.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150430/cd3b4a6e/attachment.html>