[libc-commits] [PATCH] D83931: Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double).

[Alex Brachet via Phabricator via libc-commits]

abrachet accepted this revision.
abrachet added inline comments.


================
Comment at: libc/utils/UnitTest/Test.cpp:40
+          cpp::EnableIfType<cpp::IsIntegral<T>::Value, int> N = sizeof(T) * 2>
+std::string uintToHex(T X, bool LowerCase = false) {
+  std::string s(N, '0');
----------------
lntue wrote:
> lntue wrote:
> > sivachandra wrote:
> > > abrachet wrote:
> > > > sivachandra wrote:
> > > > > Nit: Do we need the flexibility in case? If not, just remove it?
> > > > Is it more flexible? It's semantically the same as making this `std::string uintToHex(__uint128_t X, ...)` but inflates the binary.
> > > Not sure I understand your comment. My comment was about `LowerCase` not being important. We want this function to be a template because it is called with values of different `T` types but the case seems not required.
> > I also use this to display float, double, and exponent fields.  I can move the length N to a function parameter instead to reduce the generated binary.
> Removed the case.
> I can move the length N to a function parameter instead to reduce the generated binary.
If you want the different number of leading `'0'`'s based on the width of the type then this is fine.

I didn't catch this before but this function has changed semantics between patches. This now returns "00000{hex number}" not just "{hex number}" Is that really what we want or should s be initialized like `(Length, '\0')`? If it is supposed to be with '\0' then I still think that this should just take a `__uint128_t`, but if the leading 0's are what we want then this is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83931