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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Still, overall LGTM. I have some inline comments for further chiseling. Also:

1. Can you split the `UnitTest` changes into a separate patch?
2. The variable naming style in `Test.h` and `Test.cpp` is old style like `VarName`. To keep the files uniform, please follow the existing style.



================
Comment at: libc/utils/UnitTest/CMakeLists.txt:11
 target_link_libraries(LibcUnitTest PUBLIC libc_test_utils)
+set_target_properties(
+  LibcUnitTest
----------------
I would prefer not to need this, especially for tests.


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


================
Comment at: libc/utils/UnitTest/Test.cpp:124
+                 << "      Which is: " << LHS << '\n';
+    if constexpr (printHex) {
+      llvm::outs() << "In hexadecimal: " << LHSHex << '\n';
----------------
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>`


================
Comment at: libc/utils/UnitTest/Test.h:82
+            cpp::EnableIfType<cpp::IsIntegral<ValType>::Value ||
+                                  cpp::IsSame<ValType, __uint128_t>::Value ||
+                                  cpp::IsFloatingPointType<ValType>::Value,
----------------
Instead of checking for `__uint128_t`, you should extend `IsIntegral` trait to include `__uint128_t`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82997