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

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

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Feb 20, 2024

Fixes #4414.

While there's DevCom-1603671 which directly results in the reported error, the library implementation is also currently somehow broken - the initializer of _Enable_const_begin_end may be ill-formed when the value is expected to be false, which can make ranges::range<const ranges::zip_transform_view<...>> ill-formed when it should be false.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner February 20, 2024 18:08
@frederick-vs-ja frederick-vs-ja force-pushed the fix-zip_transform_view-const-iteration branch from b10549d to e814b2e Compare February 20, 2024 18:18
@frederick-vs-ja frederick-vs-ja force-pushed the fix-zip_transform_view-const-iteration branch from e814b2e to 52f8c07 Compare February 20, 2024 18:45
@@ -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.

@StephanTLavavej StephanTLavavej added bug Something isn't working ranges C++20/23 ranges labels Feb 20, 2024
@StephanTLavavej StephanTLavavej self-assigned this Feb 20, 2024
@StephanTLavavej StephanTLavavej removed their assignment Feb 22, 2024
@StephanTLavavej StephanTLavavej self-assigned this Feb 23, 2024
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 017bc35 into microsoft:main Feb 27, 2024
35 checks passed
@StephanTLavavej
Copy link
Member

🤐 🪄 😻

@frederick-vs-ja frederick-vs-ja deleted the fix-zip_transform_view-const-iteration branch February 27, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ranges C++20/23 ranges
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

<ranges>: zip_transform does not accept non const iterable ranges
3 participants