[libc-commits] [PATCH] D82997: Add implementations of fmin and fminf

[Tue Ly via Phabricator via libc-commits]

lntue marked 4 inline comments as done.
lntue added inline comments.


================
Comment at: libc/utils/UnitTest/Test.cpp:111
+
+  constexpr bool printHex = cpp::IsIntegral<ValType>::Value ||
+                            cpp::IsSame<ValType, __uint128_t>::Value ||
----------------
sivachandra wrote:
> Isn't this always true? For, `ValType` is integral or floating point type anyway?
There is also StringRef type being instantiated.


================
Comment at: libc/utils/UnitTest/Test.cpp:124
+                 << "      Which is: " << LHS << '\n';
+    if constexpr (printHex) {
+      llvm::outs() << "In hexadecimal: " << LHSHex << '\n';
----------------
sivachandra wrote:
> Instead of printing hex values conditioned on `printHex` (which I think is always `true`), WDYT of this when streaming out `LHS` and `RHS` values:
> 
> ```
> << "     Which is: " << describeValue(LHS) << '\n';
> ```
> 
> The function `describeValue` is a template function which does this:
> 
> 1. For integral types less than or of 64 bits, `describeValue` just returns the value as string.
> 2. For 128 bit integers, it returns a string of hex bits.
> 3. For floating point values, it returns a string like this: `Sign: <1|0>, Exponent bits: <hex bits>, Manstissa Bits: <hex bits>`
There are also StringRef type being instantiated.  I refactor this part and use template function describeValue to avoid constexpr in https://reviews.llvm.org/D83931


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82997