[libc-commits] [PATCH] D84726: [libc] Add implementations for sqrt, sqrtf, and sqrtl.

[Hung Dang via Phabricator via libc-commits]

hungptit added inline comments.


================
Comment at: libc/src/math/CMakeLists.txt:498
+  COMPILE_OPTIONS
+    -O2
+)
----------------
NIT: compile options are altered by CMAKE_BUILD_TYPE parameters. Do we need to specify the compile options for every entry point?


================
Comment at: libc/test/src/math/sqrt_test.cpp:23
+
+double nan = FPBits::buildNaN(1);
+double inf = FPBits::inf();
----------------
NIT: constexpr?


================
Comment at: libc/test/src/math/sqrtf_test.cpp:33
+  ASSERT_EQ(-0.0f, __llvm_libc::sqrtf(-0.0f));
+  ASSERT_EQ(nan, __llvm_libc::sqrtf(-1.0f));
+  ASSERT_EQ(1.0f, __llvm_libc::sqrtf(1.0f));
----------------
Do we plan to raise [[ https://en.cppreference.com/w/cpp/numeric/fenv/FE_exceptions | FE_INVALID ]]?


================
Comment at: libc/utils/FPUtil/SqrtLongDoubleX86.h:129
+    // Extract output
+    FPBits<T> out(0.0L);
+    out.exponent = x_exp;
----------------
Do these clang-tidy go away if we rename this variable to extractedOutput? If we give this variable a better name then the above comment is redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84726