This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add missing templated version of `std::lerp`
ClosedPublic

Authored by Quuxplusone on Dec 26 2021, 7:20 PM.

Details

Summary

Fixes https://github.com/llvm/llvm-project/issues/50806

The new implementation and new tests are both copy-pasted from hypot(a,b,c).

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Dec 26 2021, 7:20 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptDec 26 2021, 7:20 PM
jloser added a subscriber: jloser.Dec 26 2021, 7:25 PM
jloser added inline comments.
libcxx/test/std/numerics/c.math/cmath.pass.cpp
1139–1140

Why remove this? Seems unrelated?

Quuxplusone marked an inline comment as done.Dec 26 2021, 8:08 PM
Quuxplusone added inline comments.
libcxx/test/std/numerics/c.math/cmath.pass.cpp
1139–1140

This is a duplicate of old line 1119, erroneously copied down here with the three-argument overload even though it's passing only two arguments. I discovered this error when I copied these lines to use with lerp (because lerp doesn't have a two-argument overload). I can put this in a separate git commit, for sure.

Mordante requested changes to this revision.Jan 2 2022, 9:20 AM

Seems copy-pasting missed some code improvements and required changes.

libcxx/include/cmath
639

The function should be constexpr. Please make sure to also update the test.

648

This function requires C++20.

653
libcxx/test/std/numerics/c.math/cmath.pass.cpp
1171

The file libc++/test/std/numerics/c.math/c.math.lerp/c.math.lerp.pass.cpp has the other lerp tests, please move this test to that file.

1174

Please remove the empty strings and use std::is_same_v.

This revision now requires changes to proceed.Jan 2 2022, 9:20 AM
Quuxplusone marked 6 inline comments as done.

Add constexpr, and test it.
Mildly modernize the static_assert in <cmath> to remove the ""; use _IsSame while I'm at it.

Quuxplusone added inline comments.Jan 2 2022, 9:53 AM
libcxx/include/cmath
639

Good catch! Updated.

libcxx/test/std/numerics/c.math/cmath.pass.cpp
1139–1140
1174

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. :))

Mordante accepted this revision as: Mordante.Jan 3 2022, 10:01 AM

Thanks, a few minor points remaining. Otherwise then that LGTM!

libcxx/test/std/numerics/c.math/cmath.pass.cpp
1171

Please make the test constexpr and test it that way.

1174

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.

ldionne accepted this revision.Jan 3 2022, 10:50 AM

LGTM but like @Mordante said, please move the lerp tests to cmath.pass.cpp in a separate NFC commit.

This revision is now accepted and ready to land.Jan 3 2022, 10:50 AM
Quuxplusone added inline comments.Jan 3 2022, 5:26 PM
libcxx/test/std/numerics/c.math/cmath.pass.cpp
1174

In cmath.pass.cpp, we test the "shape" of each math function. These are static_asserts 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.

Mordante added inline comments.Jan 4 2022, 9:34 AM
libcxx/test/std/numerics/c.math/cmath.pass.cpp
1174

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.

Flatten c.math.lerp/c.math.lerp.pass.cpp to lerp.pass.cpp, to match abs.pass.cpp.
Add "See also" comments to both test_abs and test_lerp.
Reindent test_abs for consistency with the rest of these functions.

If CI is green, imma land this. There may be room for more thorough testing in lerp.pass.cpp, but at least test_lerp() does smoke-test the new overload (with both assert and static_assert).