[libc-commits] [PATCH] D91591: [libc] Add implementations of ldexp[f|l].

[Siva Chandra via Phabricator via libc-commits]

sivachandra marked 2 inline comments as done.
sivachandra added inline comments.


================
Comment at: libc/test/src/math/LdExpTest.h:116
+  using LdExpTest = LdExpTestTemplate<T>;                                      \
+  TEST_F(LdExpTest, SpecialNumbers) { testSpecialNumbers(&func); }             \
+  TEST_F(LdExpTest, PowersOfTwo) { testPowersOfTwo(&func); }                   \
----------------
lntue wrote:
> Isn't it going to show (LdExpTest, SpecialNumbers) for all float, double, and long double tests?
> Maybe append ##T to LdExpTest to distinguish between different data types?
Because of the space in `long double`, we cannot just append. Also, since this macro is to be used from different TUs, it does matter if all the classes have the same name. If we ever need different names, we can easily add a `suffix` argument to the macro.


================
Comment at: libc/utils/FPUtil/NormalFloat.h:122
+          if (result.mantissa & 0x1)
+            result.mantissa += 1;
+        }
----------------
lntue wrote:
> Can you test the case (S, E, M) = (0, 0x1, 0b11....1) and exp = -1?  The output should be (0, 0x1, 0b00..0).
Ah, good catch! Also means I should not be sending out patches at midnight! Fixed it now.


================
Comment at: libc/utils/FPUtil/NormalFloat.h:233
+        if (result.mantissa & 0x1)
+          result.mantissa += 1;
+      }
----------------
lntue wrote:
> Can you test with similar example as the above comment?
Same as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91591