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

P2374R4: views::cartesian_product #3561

Merged
merged 47 commits into from Mar 30, 2023

Conversation

JMazurkiewicz
Copy link
Contributor

@JMazurkiewicz JMazurkiewicz commented Mar 11, 2023

The final boss of C++23's std::views

Implemented papers:

Closes #2923.

Drive-by: replace consteval with _CONSTEVAL in views::adjacent_transform implementation.

Implementation details (please read while reviewing)

For simplicity: CPV means cartesian_product_view here.

  • Implementation of preconditions for CPV::size and CPV::iterator::distance-from functions:

    Preconditions of both of those functions require us to check if multiplication or addition is possible without overflow (signed or unsigned). Therefore this PR adds two functions for safe multiplication/addition:

    • bool _Add_with_overflow_check(x, y, &out) - adds two integers x and y without undefined behaviour, saves the result in &out, and if an overflow occured - returns true.
    • bool _Multiply_with_overflow_check(x, y, &out) - multiplies two integers x and y without undefined behaviour, saves the result in &out, and if an overflow occured - returns true. It is based on llvm::MulOverflow function (I hope this is fine, since NOTICE.txt mentions LLVM project).

    Signatures of both of those functions are based on Clang's intrinsics __builtin_add_overflow and __builtin_mul_overflow, except out parameter is a reference (not a pointer).

  • Implementation of iterator::operator+='s preconditions [range.cartesian.iterator]/19:

    Currently advacing past end is detected only when first range models ranges::sized_range. It is possible do detect it for any range Rng, but it would possibly make operator+= linear (for the last range):

    // How we can check advancement past end if Rng does not model `ranges::sized_range`:
    const auto _End = _RANGES end(_Range);
    
    // vvv THIS MIGHT BE LINEAR OPERATION vvv
    const auto _Dist = _RANGES advance(_It, static_cast<iter_difference_t<_Iter>>(_Off), _End);
    
    // If `_Dist < _Off` then it would have been "past the end" advancement (if we have used `operator+=`).
    _STL_VERIFY(_Dist == _Off, "{message}");
    
    // If we reached the end, make sure other iterators are at the begin (otherwise we are "past the end").
    if (_It == _End) {
      _STL_VERIFY(_Entire_tail_at_begin(make_index_sequence<sizeof...(_Rest)>{}), "{message}");
    }
  • Implementation of recommended practice for range_size_t and range_difference_t:

    This PR adds _Cartesian_product_optimal_size_type function, which tries to find the best unsigned integer type that satisfies recommendation [range.cartesian.view]/8:

    Recommended practice: The return type should be the smallest unsigned-integer-like type that is sufficiently wide to store the product of the maximum sizes of all the underlying ranges, if such a type exists.

    • To do so, I've added _Compile_time_max_size variable, which detects types with max size known at compile time. Currently those types are:

      • [const] span<T, N>, where N is not dynamic_extent,
      • [const] array<T, N>,
      • any range with static size function, like views::empty or views::single (ranges::tiny-range concept does similar thing),
      • ranges::ref_view and ranges::owning_view.
    • Then _Cartesian_product_max_size_bit_width<Rng> function uses this variable to find the smallest bit width for integer type that is able to hold max possible size of given range.

    • Then _Compile_time_max_size<Ranges...> function sums the results of invocation _Cartesian_product_max_size_bit_width for each range in Ranges. This sum is the minimal bit width of integer type that is able to hold sum or product of max possible sizes of all ranges in Ranges... pack.

    • Then we choose our minimal size type:

      if constexpr (_Optimal_size_type_bit_width <= 8) {
          return uint_least8_t{};
      } else if constexpr (_Optimal_size_type_bit_width <= 16) {
          return uint_least16_t{};
      } else if constexpr (_Optimal_size_type_bit_width <= 32) {
          return uint_least32_t{};
      } else if constexpr (_Optimal_size_type_bit_width <= 64) {
          return uint_least64_t{};
      } else {
          return _Unsigned128{}; // This might not be able to hold product/sum of max possible sizes
      }
    • The range_size_t of our CPV is common type of chosen minimal size type and range_size_ts of underlying ranges.

    That said, let's look at the benefits of such approach:

    // We known that max size of `arr` is always 4. Therefore we know `size_t` is able to hold result of `cartesian_product_view::size`:
    array arr{1, 2, 3, 4};
    auto v1 = views::cartesian_product(arr, arr, arr);
    // ^^^ range_size_t == size_t, range_difference_t == ptrdiff_t ^^^
    
    // We don't know what is the max size of `vec` at compile time. Therefore we have to choose bigger size type for our cartesian product:
    vector vec{1, 2, 3, 4};
    auto v2 = views::cartesian_product(vec, vec, vec);
    // ^^^ range_size_t == _Unsigned128, range_difference_t == _Signed128 ^^^

    The usage of _Compile_time_max_size may be (and should be) expanded (if we decide to use this approach). There are much more ranges with max size known at compile time, like vector<T, allocator<T>> (value returned by allocator<T>::max_size is known at compile time).

@github-actions github-actions bot added this to Work In Progress in Code Reviews Mar 11, 2023
stl/inc/ranges Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added ranges C++20/23 ranges cxx23 C++23 feature labels Mar 11, 2023
stl/inc/ranges Outdated Show resolved Hide resolved
@StephanTLavavej

This comment was marked as resolved.

@JMazurkiewicz JMazurkiewicz marked this pull request as ready for review March 13, 2023 22:11
@JMazurkiewicz JMazurkiewicz requested a review from a team as a code owner March 13, 2023 22:11
@JMazurkiewicz

This comment was marked as resolved.

@CaseyCarter CaseyCarter moved this from Work In Progress to Initial Review in Code Reviews Mar 13, 2023
@StephanTLavavej StephanTLavavej self-assigned this Mar 15, 2023
@StephanTLavavej

This comment was marked as resolved.

stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Mar 21, 2023
@StephanTLavavej

This comment was marked as resolved.

stl/inc/ranges Show resolved Hide resolved
stl/inc/ranges Show resolved Hide resolved
tests/std/tests/P2374R4_views_cartesian_product/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2374R4_views_cartesian_product/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2374R4_views_cartesian_product/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2374R4_views_cartesian_product/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2374R4_views_cartesian_product/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Finished reviewing, pushed changes for all of the issues I found - the most significant being bogus code in is_iter_swap_nothrow that was missed because nothing was exercising the codepath. With this, I think the final boss is ready for final review - thanks again! 😻 🎉

@StephanTLavavej StephanTLavavej removed their assignment Mar 21, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Mar 21, 2023
@StephanTLavavej

This comment was marked as resolved.

Code Reviews automation moved this from Final Review to Work In Progress Mar 29, 2023
@barcharcraz barcharcraz moved this from Work In Progress to Ready To Merge in Code Reviews Mar 29, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member

I've pushed a conflict-free merge with main, followed by a workaround for the notorious #1030 and a perma-workaround for the newly reported VSO-1782194 "x86chk /analyze emits bogus message 'AST_FUNCDEFN: function should be compiler-generated: __vec_copy_ctor'". The static analyzer really disliked the tuple of arrays of tuples, and it didn't serve a fundamental purpose, so I've split it up into individual arrays (and I've also simplified their initialization by providing explicit sizes).

@StephanTLavavej StephanTLavavej merged commit f1206d8 into microsoft:main Mar 30, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Mar 30, 2023
@StephanTLavavej
Copy link
Member

Final boss defeated! 🎉 😻 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature ranges C++20/23 ranges
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

P2374R4 views::cartesian_product
7 participants