<ranges>: Fix views::reverse for ranges::reverse_view lvalues
#2313
Conversation
| @@ -4167,24 +4167,24 @@ namespace ranges { | |||
|
|
|||
| template <class _Rng> | |||
| _NODISCARD static _CONSTEVAL _Choice_t<_St> _Choose() noexcept { | |||
| if constexpr (bidirectional_range<_Rng>) { | |||
There was a problem hiding this comment.
I removed the bidirectional_range<_Rng> check because
- [range.reverse.overview]/2 does not seem to have this
- this breaks the following (contrived) code:
#include <ranges>
#include <iterator>
#include <istream>
using namespace std::ranges;
using namespace std::views;
struct weird : view_interface<weird> {
int* begin();
int* end();
std::istreambuf_iterator<char> begin() const;
std::istreambuf_iterator<char> end() const;
};
int main() {
const auto v1 = weird{};
auto v2 = reverse(v1); // rejected by MSVC STL w/o this patch, accepted by libstdc++
auto v3 = reverse_view{v1}; // accepted by MSVC STL and libstdc++
}
There was a problem hiding this comment.
I must have been thinking this was bidirectional_range<remove_cvref_t<_Rng>>? Even if that is correct, checking it first seems to make the first 3 cases more expensive and has no effect on the reverse_view case. I'll put this in the "I don't know what I was thinking, but I know I was wrong" bucket. (No change requested.)
| @@ -4167,24 +4167,24 @@ namespace ranges { | |||
|
|
|||
| template <class _Rng> | |||
| _NODISCARD static _CONSTEVAL _Choice_t<_St> _Choose() noexcept { | |||
| if constexpr (bidirectional_range<_Rng>) { | |||
There was a problem hiding this comment.
I must have been thinking this was bidirectional_range<remove_cvref_t<_Rng>>? Even if that is correct, checking it first seems to make the first 3 cases more expensive and has no effect on the reverse_view case. I'll put this in the "I don't know what I was thinking, but I know I was wrong" bucket. (No change requested.)
Co-authored-by: Casey Carter <cartec69@gmail.com>
StephanTLavavej
changed the title
<ranges>: Fix views::reverse for ranges::reverse_view lvalues
|
Updated title so we can easily see what this PR does. |
| @@ -317,6 +317,12 @@ using move_only_view = test::range<Category, const int, test::Sized{is_random}, | |||
| IsCommon, test::CanCompare{derived_from<Category, forward_iterator_tag>}, | |||
| test::ProxyRef{!derived_from<Category, contiguous_iterator_tag>}, test::CanView::yes, test::Copyability::move_only>; | |||
|
|
|||
| void test_gh2312() { // COMPILE-ONLY | |||
| using X = ranges::iota_view<int, int>; | |||
| ranges::reverse_view<X> view; | |||
There was a problem hiding this comment.
This comes close to shadowing ranges::view:
Lines 2701 to 2702 in ee5a216
In general, I recommend avoiding top-level Standard identifiers. This improves codebase searchability, and makes tests quickly scannable by humans (e.g. if I see vector or string in a test, I can assume I'm looking at the Standard one).
However, as this doesn't actually shadow, no change requested - I think this is sufficiently small that a rename is not worth resetting testing.
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
Thanks for fixing this ranges bug! (I reversed this message twice, but you can't tell the difference now! 😹 🤪 🎉) |
Fixes #2312