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

[Tue Ly via Phabricator via libc-commits]

lntue marked an inline comment as done.
lntue added inline comments.


================
Comment at: libc/src/__support/UInt.h:24
 
-template <size_t Bits> class UInt {
+template <size_t Bits> struct UInt {
 
----------------
sivachandra wrote:
> 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`. 
It's just that accessing a specific `uint64_t` block is clearer with `.val[i]` vs `[i]`, especially when we have an array of `UInt`.  And with `low` and `high` functions being factored out to `number_pair.h`, there is no need for `private` members, and we can simply keep everything `public`.


================
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,
----------------
sivachandra wrote:
> 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?
Since we `UInt<128>` is required here as replacement for `__uint128_t` if not available, instantiate other `UInt` here is the most suitable, without complicating the dependency, I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137871