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

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/test/src/math/sqrt_test.cpp:23
+
+double nan = FPBits::buildNaN(1);
+double inf = FPBits::inf();
----------------
hungptit wrote:
> NIT: constexpr?
constexpr does not work with floating point yet.


================
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));
----------------
hungptit wrote:
> Do we plan to raise [[ https://en.cppreference.com/w/cpp/numeric/fenv/FE_exceptions | FE_INVALID ]]?
Floating point exceptions will be implemented in the future.  Currently we prioritize expanding functionality first, unless there are immediate requests/needs.


================
Comment at: libc/utils/FPUtil/SqrtLongDoubleX86.h:129
+    // Extract output
+    FPBits<T> out(0.0L);
+    out.exponent = x_exp;
----------------
hungptit wrote:
> 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.
I think clang-tidy wants the variables' initials to be capitalized;  it's a bit noisy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84726