Skip to content

Conversation

@StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Oct 14, 2020

Resolves #64 by implementing WG21-N4861's spaceship customization point objects specified in [cmp.alg].

Note that this is targeted at master, as it doesn't directly interact with #62 which is being developed in feature/spaceship.

  • <charconv>
    • Lift _Floating_type_traits into a central header, <type_traits>. It was the only thing using <float.h> here (for FLT_MANT_DIG etc.).
  • <type_traits>
    • Include <cstdint> needed by _Floating_type_traits. This is a very small header, "core" compatible, and it's actually surprising that <type_traits> wasn't already dragging it in.
    • Move _Floating_type_traits here. It's functionally unchanged, except that I've eliminated the <float.h> dependency by directly writing out the literal values. I believe that this makes the masks easier to understand, and I've retained the original computations as comments.
    • This also adds _Floating_type_traits<long double>, needed now but not by <charconv>.
    • <charconv> includes <xutility> includes <utility> includes <type_traits>.
    • <compare> includes (in concepts mode) <concepts> includes <type_traits>.
  • <xutility>
    • Lift std::ranges::_Choice_t and std::_Implicitly_convertible_to into a central header, <concepts>. Both will remain guarded by __cpp_lib_concepts. _Choice_t is additionally being lifted out to std.
    • <xutility> includes <utility> includes (in concepts mode) <concepts>.
  • <compare>
    • Include <bit> for bit_cast. It's a "core" header, although it does drag in <limits>.
    • Possible alternative: We could move _Bit_cast in <xutility> up to <type_traits>, except that it wants memcpy for __CUDACC__ and <type_traits> doesn't include <cstring>. Maybe we could write a raw byte-copying loop?
    • Implement the 6 CPOs.
    • I encountered 2 issues in the Standardese, which @CaseyCarter and I consider to be defects. I filed Spaceship CPO wording in [cmp.alg] needs an overhaul #1374 to track this. They have essentially zero impact for users (the "passing arguments as lvalues" is extremely difficult to observe, and is consistent with more modern Standardese elsewhere; the "block unqualified name lookup" technique is nearly impossible to observe, as I understand it, but should be clearly stated by the Standard.)
    • This "directly" implements the Standardese, using @CaseyCarter's clever _Choice/_Strategy system.
    • This implements strong and weak ordering for floating-point numbers by directly inspecting their representations; I have carefully commented each step, and written comprehensive tests.
  • <yvals_core.h>
    • P0768R1 is now fully implemented; remove the "partially implemented" comment.
    • Define the feature-test macro, guarded by __cpp_lib_concepts. Note that __cpp_lib_three_way_comparison is being defined to the lower value 201711L for P0768R1; it will be increased to the larger value 201907L for P1614R2.
  • tests/libcxx/expected_results.txt
  • tests/libcxx/skipped_tests.txt
  • tests/std/tests/VSO_0157762_feature_test_macros/test.cpp
    • Test the feature-test macro.
  • tests/std/test.lst
  • tests/std/tests/P0768R1_spaceship_cpos/env.lst
  • tests/std/tests/P0768R1_spaceship_cpos/test.cpp
    • Add comprehensive tests (requiring the concepts_matrix, but we don't need the strict concepts matrix - I was able to resolve all of the issues with /permissive mode).
    • Verifying "expression-equivalent" is a lot of work! I'm testing the return type, exception specification, and compile-time/run-time behavior. Because there are so many aspects to test, 6 different CPOs, and so many different cases that they activate, I've commented each test with the Standardese bullet point that it's testing.
    • In test_floating(), I've devised a system to test every possible "interesting" pair of floating-point values.

@StephanTLavavej StephanTLavavej added cxx20 C++20 feature spaceship C++20 operator <=> labels Oct 14, 2020
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner October 14, 2020 01:27
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Looks great, I had a tiny question about the include chain and another one regarding readability

@StephanTLavavej
Copy link
Member Author

StephanTLavavej commented Oct 15, 2020

@miscco, @CaseyCarter - I've addressed your feedback with:

  • Update general CPO properties test.
  • Filed GH issue for wording cleanups.
  • Take lvalue references in helper concepts.
  • Move helpers up to <concepts>.
  • Use requires instead of void_t.
  • Use _Choice<_Ty1&, _Ty2&> to reduce template instantiations.
  • Use else if constexpr followed by _Always_false for consistency.
  • Optimize throughput by directly invoking ADL.

I also added a couple of additional changes:

  • Extract _Can_fallback_eq_lt, rename _Can_fallback_eq_lt_twice.
  • Comment ADL usage.

@CaseyCarter CaseyCarter self-requested a review October 15, 2020 20:19
StephanTLavavej and others added 3 commits October 15, 2020 18:35
Co-authored-by: Casey Carter <cartec69@gmail.com>
Explicitly convert the (potentially user-defined) result of
ADL meow_order to meow_ordering, before converting it to the final type.
@CaseyCarter CaseyCarter removed their assignment Oct 16, 2020
Comment on lines 448 to 449
{10, bit_cast<float>(0xFFC00000u)}, // negative quiet NaN, no payload bits
{20, -numeric_limits<float>::infinity()}, // negative infinity
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need test coverage for signaling NaNs?

Note that it could be currently impossible to generate a float signaling NaN on the MSVC compiler at compile time. It emits a quiet NaN for bit_cast<float>(0xFF800001u) or numeric_limits<float>::signaling_NaN().

Same below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, yes, this would test signaling NaNs - but as you noted, the compiler bugs make it difficult. I think we can live without test coverage for them, given that the code is clearly written to handle them uniformly (the main question is whether I got the "infinity plus one" logic correct, but I think that can be verified by inspecting the code).

All this header does is include `stdint.h` and provide typedefs.
@StephanTLavavej

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@StephanTLavavej StephanTLavavej merged commit 1469bf3 into microsoft:master Oct 29, 2020
@StephanTLavavej StephanTLavavej deleted the ufo_cpos branch October 29, 2020 07:49
@StephanTLavavej StephanTLavavej removed their assignment Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature spaceship C++20 operator <=>

Projects

None yet

Development

Successfully merging this pull request may close these issues.

P0768R1 Library Support For The Spaceship Comparison Operator <=>

5 participants