[libc-commits] [PATCH] D124959: [libc] add uint128 implementation

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

In D124959#3497759 <https://reviews.llvm.org/D124959#3497759>, @michaelrj wrote:

> I'm not sure that extending `UInt.h` is a good idea, since it adds a lot of complexity for not a lot of benefit.

If you are starting out, and need only 128 bit integers, I agree with this. The point I am making is, a much general class is already present.

> If we had actual uses for integers larger than 128 bits, then I would agree, but right now the `UInt` class is not used in any entrypoint.

I agree its not used in an entrypoint. But, that doesn't mean its incorrect or totally unused. It is actually tested in one range reduction function.

> To move these functions to `UInt` would involve rewriting all of these functions to support arbitrary sized integers when in reality there's only one size used.

My point is to not move any of the complex pieces. Those complex parts are already implemented in very a general fashion.

> Finally, The designs of these classes are different, my `UInt128` class is intended to be an immutable number, acting similar to a primitive.

That is why I said that all you need is to add a few straightforward methods. Before I actually made the comment, I tried it out to actually convince myself that I was talking sense. With just a few additional but straightforward methods, all your tests work as is. For example, for addition, all I had to add was this:

  UInt<Bits> operator+(const UInt<Bits> &other) const {
    UInt<Bits> result(*this);
    result.add(other);
    return result;
  }

Similarly, I added straightforward methods for multiplication, shift and bitwise operators also.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124959/new/

https://reviews.llvm.org/D124959