microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
9.94k stars 1.46k forks source link

`<ranges>`: `take_view`/`drop_view`/`slide_view`'s `range_difference_t<const V>` issue #2894

Open hewillk opened 1 year ago

hewillk commented 1 year ago

https://github.com/microsoft/STL/blob/04ee87830a404437f1691d6a3e8809e4986d8493/stl/inc/ranges#L2831-L2836

Since range_difference_t<V> is not guaranteed to be the same as range_difference_t<const V>, we'd better do type casting before calling std::min. The standard has no such problem because it always calls ranges​::​next(ranges​::​begin(base_), count_, ranges​::​end(base_)), and range_difference_t<const V> is guaranteed to be implicitly converted to range_difference_t<V>because they are both signed.

#include <ranges>

auto s = std::views::single(0ULL);
auto o = std::views::iota(0ULL, 1ULL);

struct R {
  auto begin() { return s.begin(); }
  auto end() { return s.end(); }
  auto begin() const { return o.begin(); }
  auto end() const { return o.end(); }
};

int main() {
  const auto d = R{} | std::views::drop(1);
  auto b = d.begin();
}

https://godbolt.org/z/Pj5cP394n

frederick-vs-ja commented 1 year ago

While this bug should be fixed (by using static_cast<range_difference_t<_Vw>>(_RANGES distance(_Range)) instead of _RANGES distance(_Range), IMO), I have some further concerns.

In some cases, range_difference_t<const V> is wider than range_difference_t<V>, and the number of elements of the const-qualified adopted view may actually more than numeric_limits<range_difference_t<V>>::max(). But drop_view is equivalently required to only store a range_difference_t<V> object, which disallows dropping a huge number of elements from the const-qualified adopted view.

Should we file an LWG issue to change the status quo, even though it's probably ABI-breaking?

Edit: take_view and slide_view also suffer from this problem.

hewillk commented 1 year ago

In some cases, range_difference_t<const V> is wider than range_difference_t<V>

Then the const begin/end of slide_view may also have bugs

https://github.com/microsoft/STL/blob/04ee87830a404437f1691d6a3e8809e4986d8493/stl/inc/ranges#L6080-L6084 https://github.com/microsoft/STL/blob/04ee87830a404437f1691d6a3e8809e4986d8493/stl/inc/ranges#L6112-L6121

There may be a narrowing conversion error for {_Count}, and noexcept(_RANGES begin(_Range) + _Count)) is not strictly correct because _RANGES begin(_Range) is not guaranteed to work with _Count. (Since you suggested that we should group these bugs into a meta bug so I'm not going to open an new issue)

I think a paper might be needed to specify the relationship between range_difference_t<V> and range_difference_t<const V>, which should belong to the same category of issues as LWG3564. Not sure if Casey's paper mentions this.

hewillk commented 1 year ago

https://github.com/microsoft/STL/blob/04ee87830a404437f1691d6a3e8809e4986d8493/stl/inc/ranges#L2470-L2480

This should be static_cast<range_difference_t<const _Vw>>(size()) as well, which should have been resolved after implementing P2393.

CaseyCarter commented 1 year ago

I have work-in-progress to address this.

hewillk commented 1 year ago

You might also consider issues like the implicit conversion of iter_difference_t<O> to ptrdiff_t (which may not be well-formed) in <format>.

hewillk commented 2 months ago

There is a similarly contrived issue about slide_view. https://godbolt.org/z/Ksveer6qq