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.

Sign up for GitHub

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

<ranges>: Fix zip_transform_view by not using possibly ill-formed static data member #4416

Merged
merged 1 commit into from
Feb 27, 2024
+24 −5
Merged

<ranges>: Fix zip_transform_view by not using possibly ill-formed static data member #4416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions stl/inc/ranges
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8582,6 +8582,10 @@ namespace ranges {
_EXPORT_STD inline constexpr _Zip_fn zip{};
} // namespace views

template <class _Func, class... _ViewTypes>
concept _Can_const_iterate_zip_transform_view =
range<const zip_view<_ViewTypes...>> && regular_invocable<const _Func&, range_reference_t<const _ViewTypes>...>;

_EXPORT_STD template <class _Func, class... _ViewTypes>
requires _Zip_transform_constraints<_Func, _ViewTypes...>
class zip_transform_view : public view_interface<zip_transform_view<_Func, _ViewTypes...>> {
Expand Down Expand Up @@ -8833,9 +8837,6 @@ namespace ranges {
}
}

static constexpr bool _Enable_const_begin_end =
(range<const _Inner_view> && regular_invocable<const _Func&, range_reference_t<const _ViewTypes>...>);

public:
zip_transform_view() = default;

Expand All @@ -8849,7 +8850,7 @@ namespace ranges {
}

_NODISCARD constexpr auto begin() const noexcept(noexcept(_Iterator<true>{*this, _Zip.begin()})) // strengthened
requires _Enable_const_begin_end
requires _Can_const_iterate_zip_transform_view<_Func, _ViewTypes...>
{
return _Iterator<true>{*this, _Zip.begin()};
}
Expand All @@ -8863,7 +8864,7 @@ namespace ranges {
}

_NODISCARD constexpr auto end() const noexcept(_Is_end_noexcept<true>()) // strengthened
requires _Enable_const_begin_end
requires _Can_const_iterate_zip_transform_view<_Func, _ViewTypes...>
{
if constexpr (common_range<const _Inner_view>) {
return _Iterator<true>{*this, _Zip.end()};
Expand Down
18 changes: 18 additions & 0 deletions tests/std/tests/P2321R2_views_zip_transform/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,20 @@ constexpr bool validate_empty_ranges() {
return true;
}

// Also test GH-4414: "<ranges>: zip_transform does not accept non const iterable ranges"
constexpr bool test_gh_4414() {
auto evens_and_odds = views::zip_transform([](int even, int odd) { return even + odd; },
views::iota(0, 10) | views::filter([](int i) { return i % 2 == 0; }),
views::iota(0, 10) | views::filter([](int i) { return i % 2 != 0; }));

using ZippedTransformed = decltype(evens_and_odds);
STATIC_ASSERT(ranges::range<ZippedTransformed>);
STATIC_ASSERT(!ranges::range<const ZippedTransformed>);

constexpr int expected_results[]{1, 5, 9, 13, 17};
return ranges::equal(expected_results, evens_and_odds);
}

int main() {
// Empty RangeTypes... parameter pack
{
Expand Down Expand Up @@ -858,6 +872,10 @@ int main() {
test_one(three_element_transform_closure, three_range_transform_results_array, test_element_array_one,
test_element_array_two, test_element_array_three);
}
{
STATIC_ASSERT(test_gh_4414());
Copy link
Contributor

Choose a reason for hiding this comment

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

(Pre-existing) I'm wondering why would C++20 tests use STATIC_ASSERT macro rather than terse static_assert

Copy link
Member

Choose a reason for hiding this comment

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

It stringifies the condition. MSVC doesn't report the condition (/diagnostics:caret reports it indirectly, but we currently don't use that in our tests - I think we did in the past):

C:\Temp>type woof.cpp
static_assert(17 == 29);

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /c woof.cpp
woof.cpp
woof.cpp(1): error C2607: static assertion failed

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /c /diagnostics:caret woof.cpp
woof.cpp
woof.cpp(1,18): error C2607: static assertion failed
static_assert(17 == 29);
                 ^

C:\Temp>clang-cl /EHsc /nologo /W4 /std:c++latest /c woof.cpp
woof.cpp(1,15): error: static assertion failed due to requirement '17 == 29'
    1 | static_assert(17 == 29);
      |               ^~~~~~~~
1 error generated.

IIRC, other maintainers liked the stringifying, whereas I didn't care - unlike runtime assertions (which can be sporadic, so it's helpful to capture in logs what failed, even if it's technically possible to go look up line numbers), static_asserts tend not to start failing, or if they do it's because of constexpr evaluation in which case the line isn't actually interesting, so I have to build the test case myself anyways, in which case I can see the line right there.

I think the status quo is that some, but not all, C++20-and-above tests use the STATIC_ASSERT macro while others use terse static_assert, reflecting the strength of maintainer feelings about this.

assert(test_gh_4414());
}

return 0;
}
Loading