Skip to content

<xutility>: Some fixes around std::ranges::enable_view#2978

Merged
StephanTLavavej merged 4 commits into
microsoft:mainfrom
frederick-vs-ja:patch-enable_view
Aug 3, 2022
Merged

<xutility>: Some fixes around std::ranges::enable_view#2978
StephanTLavavej merged 4 commits into
microsoft:mainfrom
frederick-vs-ja:patch-enable_view

Conversation

@frederick-vs-ja
Copy link
Copy Markdown
Contributor

@frederick-vs-ja frederick-vs-ja commented Aug 1, 2022 •
edited
Loading

Currently the implementation of std::ranges::enable_view in MSVC STL doesn't reject reference types.

static_assert(std::ranges::enable_view<std::ranges::empty_view<int>&>); // currently passes, but should not

And it doesn't handle incomplete types whenever possible due to ADL.

struct Incomplet;

template <class T>
struct Holder {
    T t;
};

static_assert(!std::ranges::enable_view<Holder<Incomplet>*>); // hard error, should be avoided

These bugs are revealed by the libc++ test std/ranges/range.req/range.view/enable_view.compile.pass.cpp.

The fixes are

  • requiring the type to be an object type in the derive-from-view_interface mechanism, and
  • using qualified call to avoid ADL.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner August 1, 2022 03:49
@CaseyCarter CaseyCarter added bug Something isn't working ranges C++20/23 ranges labels Aug 1, 2022
strega-nil-ms
strega-nil-ms reviewed Aug 1, 2022
View reviewed changes
Comment thread tests/std/tests/P0896R4_ranges_range_machinery/test.cpp
STATIC_ASSERT(!ranges::enable_view<const strange_view4&&>);

// Verify that the derived-from-view_interface mechanism can handle uses of incomplete types whenever possible
struct incomplet;
Copy link
Copy Markdown
Contributor

@strega-nil-ms strega-nil-ms Aug 1, 2022

Choose a reason for hiding this comment

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

Why the mis-spelling of incomplete here?

Copy link
Copy Markdown
Contributor

@CaseyCarter CaseyCarter Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The word "incomplete" is incomplete. I assume it's intentional (See "Note: this is an early draft. It’s known to be incomplet and incorrekt, and it has lots of bad formatting.").

Copy link
Copy Markdown
Member

@StephanTLavavej StephanTLavavej Aug 2, 2022

Choose a reason for hiding this comment

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

It's a little bit silly but I think it's acceptable.

Copy link
Copy Markdown
Contributor Author

@frederick-vs-ja frederick-vs-ja Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is incorrekt incorrect but consistent with the C++ Working Draft.😺

StephanTLavavej
StephanTLavavej reviewed Aug 1, 2022
View reviewed changes
Comment thread stl/inc/xutility Outdated
Comment thread tests/std/tests/P0896R4_ranges_range_machinery/test.cpp
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Aug 1, 2022

Thanks, this looks good! I pushed very minor changes (product code comment, additional test line).

CaseyCarter
CaseyCarter approved these changes Aug 1, 2022
View reviewed changes
Comment thread stl/inc/xutility Outdated
@StephanTLavavej StephanTLavavej self-assigned this Aug 3, 2022
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Aug 3, 2022

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

@StephanTLavavej StephanTLavavej merged commit 09d5ac7 into microsoft:main Aug 3, 2022
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Aug 3, 2022

Thanks for investigating and fixing these bugs! 🔍 🐞 😻

@frederick-vs-ja frederick-vs-ja deleted the patch-enable_view branch August 3, 2022 23:32
strega-nil pushed a commit to strega-nil/stl that referenced this pull request Aug 6, 2022
@frederick-vs-ja @StephanTLavavej @strega-nil
<xutility>: Some fixes around std::ranges::enable_view (microsoft…
90d161e 
…#2978)

Co-authored-by: Stephan T. Lavavej <[email protected]>
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
@frederick-vs-ja @StephanTLavavej @fsb4000
<xutility>: Some fixes around std::ranges::enable_view (microsoft…
6f96cb0 
…#2978)

Co-authored-by: Stephan T. Lavavej <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+2 more reviewers

@strega-nil-ms strega-nil-ms strega-nil-ms left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

bug Something isn't working ranges C++20/23 ranges

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@frederick-vs-ja @StephanTLavavej @CaseyCarter @strega-nil-ms