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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added a comment.
This revision is now accepted and ready to land.

The tests look so much more readable now. Thanks for patiently working through the comments.
Just a note for future: We should setup differential fuzzers for math functions comparing LLVM libc results with results from system libc like glibc. And may be also a doc listing how we differ from glibc and the justification.

I have accepted this revision, but I guess you need to wait for the other patch to land before you can submit this. Also, I left couple nits inline. Please fix them if relevant.



================
Comment at: libc/src/math/fminf.cpp:19
+} // namespace __llvm_libc
\ No newline at end of file

----------------
Newline?


================
Comment at: libc/test/src/math/fmin_test.cpp:18
+
+double nan = static_cast<double>(FPBits::buildNaN(1));
+double inf = static_cast<double>(FPBits::inf());
----------------
Here, and in the other test files, I don't think you need the `static_cast`. We have implicit conversion operators for `FPBits`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82997