[libc-commits] [PATCH] D90906: [libc] Add implementations of fdim[f|l].

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/test/src/math/FDimTest.h:15
+
+namespace __llvm_libc {
+namespace testing {
----------------
To be consistent with other places, do not put the test classes in the any namespace.


================
Comment at: libc/test/src/math/FDimTest.h:24
+  void testNaNArg(FuncPtr func) {
+    DECLARE_SPECIAL_CONSTANTS(T)
+    EXPECT_FP_EQ(nan, func(nan, inf));
----------------
Can you move this declaration out of the method? This way, other methods can also use them without redeclarations?


================
Comment at: libc/test/src/math/fdim_test.cpp:17
+
+using RunContext = __llvm_libc::testing::RunContext;
+
----------------
We don't need these. I will clean it up in the ilogb tests.


================
Comment at: libc/utils/FPUtil/BasicOperations.h:78
+
+  return (x > y ? x - y : 0);
+}
----------------
The operation `x - y` can lead to overflow or underflow. Shouldn't we detect that and return the appropriate value as specified by the standard? Or, should we assume that the machine instruction does the right thing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90906