[libcxx-commits] [PATCH] D116295: [libc++] Add missing templated version of `std::lerp`

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/test/std/numerics/c.math/cmath.pass.cpp:1180
+#if TEST_STD_VER > 17
+    static_assert((std::is_same<decltype(std::lerp((float)0, (float)0, (float)0)), float>::value), "");
+    static_assert((std::is_same<decltype(std::lerp((float)0, (bool)0, (float)0)), double>::value), "");
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > Mordante wrote:
> > > > Please remove the empty strings and use `std::is_same_v`.
> > > > 
> > > Both here and above, this is for consistency with everything else in this file. Trying to "do something different" by creating a new `c.math.lerp.pass.cpp` is how we ended up missing tests (and code) to begin with; mclow should have just copy-pasted all the stuff for `std::hypot` (as I did here) and then the template overload couldn't have been missed.
> > > (Although, as you see, the `constexpr` can be missed if I copy from `hypot`-which-is-not-constexpr. :))
> > I've no strong opinion whether `lerp` gets it's own file or becomes part of this test. I object against having the tests for `lerp` in two different files. If you want to keep it in this file, please make add the current `lerp` tests to this file in a separate commit.
> In `cmath.pass.cpp`, we test the "shape" of each math function. These are `static_assert`s about the `decltype` of various calls, to prove we got the shape right; and then just a few lines of sanity-checks on the behavior (because we assume libc gets the behavior basically right, so the sanity-check is all we need).
> 
> The trick with `lerp` is that it has the same "shape" as the other cmath functions (like `ilogb` and `lgamma` and `hypot`), but whereas those just call out to libc, `lerp` is actually implemented directly in C++, so we need extra tests for its behavior. In this respect, `lerp` is like `abs`.
> 
> `libcxx/test/std/numerics/c.math/c.math.lerp/c.math.lerp.pass.cpp` is analogous to `libcxx/test/std/numerics/c.math/abs.pass.cpp`. It's specifically to test `lerp`'s //behavior// because we implemented it ourselves and can't rely on libc to get it right.
> 
> What I can do, though, is simplify/expand `c.math.lerp.pass.cpp` to be more thorough, use our current style for constexpr-friendly tests, and test the generic template overload too. I would also be delighted to rename it to  `libcxx/test/std/numerics/c.math/lerp.pass.cpp`, for parallelism with `abs.pass.cpp`; just like D116198 did for the range.access stuff.
Having a basic test here and a more verbose in the renamed `lerp.pass.cpp` works for me.
I would prefer to add some comment at both places referring to the other test, so the reader is aware of the other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116295