[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/utils/UnitTest/LibcTest.cpp:281
 
+template bool test<__llvm_libc::cpp::UInt<192>>(
+    RunContext *Ctx, TestCondition Cond, __llvm_libc::cpp::UInt<192> LHS,
----------------
lntue wrote:
> sivachandra wrote:
> > lntue wrote:
> > > sivachandra wrote:
> > > > lntue wrote:
> > > > > 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.
> > > > We can keep specializations of only standard types here. Everything else, including those for `__uint128_t`, should go to another file.
> > > Does it also mean that by default we will need to link to the other file, in addition to `LibcTest` ?
> > Yes, like other custom matchers we have.
> The template `test` that is instantiated here is actually only defined internally inside `LibcTest.cpp`. Untangle and refactoring it out to a common header so that we can have a separate instantiation target for `UInt<192>` and `UInt<256>` feel like out-of-scope for this patch.  It should be a separate task / patch to refactor this file instead.
Yes - what I am describing is the end situation. You don't have to do it in this patch. The main point is that we can't keep adding specializations here. I agree it will take much more surgery than just moving it out. It is probably also exposing a deficiency in the current design.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137871