STL icon indicating copy to clipboard operation
STL copied to clipboard

`<ranges>`: `lazy_split_view` should use `_Non_propagating_cache` instead of `_Defaultabox`

Open cpplearner opened this issue 1 year ago • 2 comments
trafficstars

Revealed by libc++ tests std/ranges/range.adaptors/range.lazy.split/ctor.copy_move.pass.cpp and std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/iter_swap.pass.cpp.

[range.lazy.split.view] says lazy_split_view<V, Pattern> stores a non-propagating-cache<<iterator_t<V>> if forward_range<V> is false, but MSVC STL's lazy_split_view stores a _Defaultabox:

https://github.com/microsoft/STL/blob/a8888806c6960f1687590ffd4244794c753aa819/stl/inc/ranges#L4612-L4616

This makes lazy_split_view non-copyable when iterator_t<V> is non-copyable.

#include <ranges>
#include <cassert>
using namespace std;

struct MoveOnlyIter {
    using difference_type = int;
    using value_type = int;

    MoveOnlyIter(MoveOnlyIter&&);
    MoveOnlyIter& operator=(MoveOnlyIter&&);

    int operator*() const;
    MoveOnlyIter& operator++();
    void operator++(int);

    bool operator==(int) const;
};

struct TestRange {
    MoveOnlyIter begin();
    int end();
};

void test() {
    TestRange r;
    auto lsv = r | std::views::lazy_split(0);
    auto lsv2 = lsv;  // error on MSVC, should be OK
}

This seems easy to fix, but might require an ABI-breaking change: both _Defaultabox and _Non_propagating_cache have space-efficient specializations, but with different constraints, which means they may have different layout in some cases.

cpplearner avatar Dec 17 '23 11:12 cpplearner

Perhaps the use of _Defaultabox in join_view's iterator is also problematic, and we should completely get rid of _Defaultabox.

Edit: yes, these 3 tests pass if we change to use optional:

  • std/ranges/range.adaptors/range.join/end.pass.cpp
  • std/ranges/range.adaptors/range.join/range.join.sentinel/ctor.parent.pass.cpp
  • std/ranges/range.adaptors/range.join/range.join.sentinel/eq.pass.cpp

frederick-vs-ja avatar Dec 17 '23 15:12 frederick-vs-ja

Thanks for analyzing this bug! We talked about this at the weekly maintainer meeting and we believe we can get away with changing these types, not worrying about the ABI implications in extreme corner cases.

StephanTLavavej avatar Jan 10 '24 22:01 StephanTLavavej