Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish P0811R3 midpoint and lerp #1048

Merged
merged 12 commits into from
Jul 30, 2020
Merged

Conversation

statementreply
Copy link
Contributor

Resolves #65.

* Removes workaround for missing `bit_cast()` and mark `lerp()` constexpr.

* Changes how `lerp()` handles infinity inputs according to
  microsoft#65 (comment) and
  microsoft#65 (comment).
* Adds constexpr tests.

* Updates test cases for infinity inputs according to the new behavior.
@statementreply statementreply requested a review from a team as a code owner July 16, 2020 15:46
clang-format has seemingly changed its mind on some unrelated code :\
@CaseyCarter CaseyCarter added the cxx20 C++20 feature label Jul 16, 2020
stl/inc/cmath Show resolved Hide resolved
stl/inc/cmath Outdated Show resolved Hide resolved
return _Val2;
}
} else {
if (_STD _Is_nan(_Val1) || _STD _Is_nan(_Val2)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I purposely put this test below the high limit tests before the NaN tests because we expect NaNs to be uncommon; you're fixing -fassociative-math and friends but it seems like that would still be fixed if you put this in the old location?

Copy link
Contributor Author

@statementreply statementreply Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performing the high limit test with <= raises undeserved FE_INVALID when the input value is NaN.

Alternative approaches:

  • Test with quiet comparison islessequal
  • Test with bit_cast tricks
  • Just ignore the issue of FE_INVALID

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, to raise floating point exceptions. Can you add a comment to places where you do seemingly needless operations to raise the right exceptions and add tests for that? Otherwise that's likely to break. (I understand that requires /fp:except which might be annoying to set up in our tests :( )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm actually it looks like we already have an /fp:except flavor tested:

PM_CL="/EHsc /MDd /D_ITERATOR_DEBUG_LEVEL=2 /std:c++latest /fp:except /Zc:preprocessor"

so you should just be able to do that.

@StephanTLavavej

This comment has been minimized.

@statementreply
Copy link
Contributor Author

statementreply commented Jul 23, 2020

libcxx test for lerp is still failing. I believe this is a bug in the test.

https://github.com/llvm/llvm-project/blob/d66428cb995c7a04ecb02951d5021d815fc02b2b/libcxx/test/std/numerics/c.math/c.math.lerp/c.math.lerp.pass.cpp#L56-L59

The test asserts lerp(2.3, 2.3, inf) == 2.3 and isnan(lerp(0.0, 0.0, inf)). There's no reason why they should behave differently.

Copy link
Contributor

@cbezault cbezault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

stl/inc/cmath Outdated Show resolved Hide resolved
@cbezault cbezault removed their assignment Jul 29, 2020
@CaseyCarter CaseyCarter self-assigned this Jul 29, 2020
@CaseyCarter CaseyCarter changed the title Finish P0811R3 midpoint(), lerp() Finish P0811R3 midpoint and lerp Jul 29, 2020
It's technically nonconformant to explicitly specialize a non-inline
variable template (and _INLINE_VAR is inline for C++17 and above). MSVC
accepts this, but it causes problems for Clang.
tests/std/tests/Dev09_172666_tr1_tuple_odr detected this in the
MSVC-internal repo, but not on GitHub due to an oversight that we just
discovered and are fixing (it has one.cpp and two.cpp files instead of
test.cpp, so we need to add a custom run script).

We can fix this by just using a struct template with static constexpr
data members.

A couple of other changes here: Use is_floating_point_v instead
of ::value, and _Float_abs_bits doesn't need to static_cast at the end
(the inputs to bitwise-AND are the same type and are unsigned int or
unsigned long long, so there's no integral promotions involved).

cmath: Add Oxford comma to comment.

numeric: Now that we require is_integral_v, _Arithmetic should be
renamed to _Integral.
@StephanTLavavej
Copy link
Member

Pushed a change to fix a test failure that we just encountered:

Fix a subtle conformance issue.

It's technically nonconformant to explicitly specialize a non-inline variable template (and _INLINE_VAR is inline for C++17 and above). MSVC accepts this, but it causes problems for Clang. tests/std/tests/Dev09_172666_tr1_tuple_odr detected this in the MSVC-internal repo, but not on GitHub due to an oversight that we just discovered and are fixing (it has one.cpp and two.cpp files instead of test.cpp, so we need to add a custom run script).

We can fix this by just using a struct template with static constexpr data members.

A couple of other changes here: Use is_floating_point_v instead of ::value, and _Float_abs_bits doesn't need to static_cast at the end (the inputs to bitwise-AND are the same type and are unsigned int or unsigned long long, so there's no integral promotions involved).

cmath: Add Oxford comma to comment.

numeric: Now that we require is_integral_v, _Arithmetic should be renamed to _Integral.

@CaseyCarter CaseyCarter merged commit e9f56a6 into microsoft:master Jul 30, 2020
@CaseyCarter
Copy link
Member

Thanks for increasing t all the way to 1.0 for midpoint and lerp!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P0811R3 midpoint(), lerp()
5 participants