[PATCH] Fix private constructor for ScaledNumber.

[Duncan P. N. Exon Smith]

> On 2015-Apr-29, at 13:32, Diego Novillo <dnovillo at google.com> wrote:
> 
> Hi dexonsmith,
> 
> It was using uint64_t instead of DigitsT. This was preventing
> instantiations of ScaledNumber with anything other than uint64_t types.
> 
> In implementing the tests, I ran into another issue. Operators >>= and
> <<= did not have variants for accepting other ScaledNumber as the shift
> argument. This is expected by the SCALED_NUMBER_BOP.
> 
> I'm not sure it makes sense to allow shifting a ScaledNumber by another
> ScaledNumber, so I generated a new template for shifting ScaledNumbers
> using a new macro SCALED_NUMBER_SHIFT.
> 
> http://reviews.llvm.org/D9350
> 
> Files:
>  include/llvm/Support/ScaledNumber.h
>  unittests/Support/ScaledNumberTest.cpp

LGTM, with some minor comments below.  Thanks for the fixes!

> Index: include/llvm/Support/ScaledNumber.h
> ===================================================================
> --- include/llvm/Support/ScaledNumber.h
> +++ include/llvm/Support/ScaledNumber.h
> @@ -514,7 +514,7 @@
>        : Digits(Digits), Scale(Scale) {}
>  
>  private:
> -  ScaledNumber(const std::pair<uint64_t, int16_t> &X)
> +  ScaledNumber(const std::pair<DigitsT, int16_t> &X)
>        : Digits(X.first), Scale(X.second) {}
>  
>  public:
> @@ -732,10 +732,18 @@
>  SCALED_NUMBER_BOP(-, -= )
>  SCALED_NUMBER_BOP(*, *= )
>  SCALED_NUMBER_BOP(/, /= )
> -SCALED_NUMBER_BOP(<<, <<= )
> -SCALED_NUMBER_BOP(>>, >>= )
>  #undef SCALED_NUMBER_BOP
>  
> +#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.

> +
>  template <class DigitsT>
>  raw_ostream &operator<<(raw_ostream &OS, const ScaledNumber<DigitsT> &X) {
>    return X.print(OS, 10);
> Index: unittests/Support/ScaledNumberTest.cpp
> ===================================================================
> --- unittests/Support/ScaledNumberTest.cpp
> +++ unittests/Support/ScaledNumberTest.cpp
> @@ -155,7 +155,7 @@
>    EXPECT_EQ(SP32(0xaaaaaaab, -33), getQuotient32(1, 3));
>    EXPECT_EQ(SP32(0xd5555555, -31), getQuotient32(5, 3));
>  
> -  // 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.

>    // 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?

> +  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.

> +  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.

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

> +}
> +
>  } // end namespace
>