This is an archive of the discontinued LLVM Phabricator instance.

Integer and pointer types of 'midpoint' from P0811
ClosedPublic

Authored by mclow.lists on Mar 7 2019, 9:13 AM.

Details

Reviewers
EricWF
ldionne
Summary

https://wg21.link/P0811

This is just the integer and pointer parts of std::midpoint.
The floating point bits and the lerp will come later.'

Diff Detail

Event Timeline

mclow.lists created this revision.Mar 7 2019, 9:13 AM
EricWF added inline comments.Mar 8 2019, 8:47 PM
libcxx/include/numeric
533

Does const or volatile occur for by-value template parameters?

551

Why the SFINAE?

Just declare it midpoint(_Tp*, _Tp*) ?

libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.integer.pass.cpp
29

How about cases where the sum is odd? AKA 0, 3, 3, 0, 4, -1, and -1, 4).

32

How about all these same tests but with -2 instead?

mclow.lists marked 5 inline comments as done.Mar 12 2019, 1:33 PM
mclow.lists added inline comments.
libcxx/include/numeric
533

I can remove that now that I realize that users are not supposed to explicitly list the template parameters.

551

Because of this bad thing:

midpoint<int>(0, 0)

returns a int *

Yes, I know that users are not supposed to do that, but this is easy prevention.

libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.integer.pass.cpp
29

I have more down below.

mclow.lists marked 2 inline comments as done.

Address Eric's comments.
Remove all the explicit template lists from the tests.
Add a couple "odd" tests.
Add a couple more failing pointer tests.

EricWF accepted this revision.Mar 13 2019, 8:12 PM

LGTM minus inline comments.

libcxx/include/numeric
529

TODO on the floating point overloads?

549

This overload isn't constexpr in the paper.

551

The int case isn't quite right, but midpoint<long>(0, 0) triggers the bug you mention.

Is there a test case?

553

minpoint needs to be qualified.

libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.integer.pass.cpp
31

There's no reason why midpoint needs to be declared as a template. This test is non-portable.

64

I think we need more constexpr test that try to tickle overflows and other boundary conditions to ensure we're constexpr in those cases as well.

This revision is now accepted and ready to land.Mar 13 2019, 8:12 PM
EricWF added inline comments.Mar 13 2019, 11:44 PM
libcxx/include/numeric
549

Ignore me. It is in r3.

libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.integer.pass.cpp
31

Ignore me. It is in r3.

mclow.lists closed this revision.Mar 29 2019, 5:55 PM
mclow.lists marked 2 inline comments as done.

Landed as r356162

libcxx/include/numeric
553

I changed this to be: return __a + _VSTD::midpoint(ptrdiff_t(0), __b - __a);

zoecarver added inline comments.
libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.fail.cpp
19

Is this necessary?

libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.integer.pass.cpp
29

nit: any reason not to use`ASSERT_SAME_TYPE`?

mclow.lists marked 5 inline comments as done.Mar 30 2019, 1:28 PM
mclow.lists added inline comments.
libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.integer.pass.cpp
29

I actually changed that before I committed.