[libc-commits] [PATCH] D137871: [libc][math] Improve the performance and error printing of UInt.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/UInt.h:24
 
-template <size_t Bits> class UInt {
+template <size_t Bits> struct UInt {
 
----------------
lntue wrote:
> michaelrj wrote:
> > is there a reason this is changed?
> I put everything in this class as `public` for 3 reasons:
> - To access to `WordCount` without recomputing it
> - To make this class behave as a value class, I'm going to remove the `operator[]`, and simply use `.val[i]` directly if we want to access to its ith `uint64_t` block.
> - Utility functions `low` and `high` (and `MASK32` constant) will be removed and switched completely to use `split(uint64_t)` instead.  That would separate what we want to do with the class vs what we want to do with `uint64_t`.
> I put everything in this class as `public` for 3 reasons:
> - To access to `WordCount` without recomputing it

You can make that public alone?

> - To make this class behave as a value class, I'm going to remove the `operator[]`, and simply use `.val[i]` directly if we want to access to its ith `uint64_t` block.

Whats wrong with `operator[]` for such word access?

> - Utility functions `low` and `high` (and `MASK32` constant) will be removed and switched completely to use `split(uint64_t)` instead.  That would separate what we want to do with the class vs what we want to do with `uint64_t`.

I did not get how this is related to `class` vs `struct`. 


================
Comment at: libc/src/__support/number_pair.h:37
+
+template <typename T> number_pair<T> full_mul(T a, T b);
+
----------------
The `full_mul` functions should not belong here or name this file something else. May be `integer_utils.h`.


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:281
 
+template bool test<__llvm_libc::cpp::UInt<192>>(
+    RunContext *Ctx, TestCondition Cond, __llvm_libc::cpp::UInt<192> LHS,
----------------
Ideally, these specializations should be in a different file, say `integer_test_utils.cpp`, right next to where `UInt` is defined. May be it leads to a CMake cyclic directory problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137871