Skip to content

Conversation

theuni
Copy link
Member

@theuni theuni commented Oct 8, 2021

This is a c++20 feature, so it's obviously not a rush to get in, but it may be useful while we play around with ranges in the codebase before actually switching to c++20.

This matches the behavior of std::span.

The below sample program demonstrates what these enable:

#include <vector>
#include <ranges>
#include <cstdio>
#include "span.h"

static std::vector<int> g_vec{1, 2, 3, 4, 5, 6};

void span_view_filter() {
    auto even = [](int i) { return 0 == i % 2; };
    printf("evens: ");
    for (int i : Span<int>(g_vec) | std::views::filter(even)) {
        printf("%i ", i);
    }
    printf("\n");
}

void span_no_dangle()
{
    auto get_span = [] {
        return Span<int>(g_vec);
    };
    auto iter = std::ranges::max_element(get_span());
    static_assert(std::is_same_v<int*, decltype(iter)>);
    printf("max: %i\n", *iter);
}

int main()
{
    span_no_dangle();
    span_view_filter();
}

Compiled with: g++-10 -O2 -std=c++2a span.cpp -o span

If enable_view is disabled, the first example will fail to compile.
If enable_borrowed_range is disabled, the second will fail to compile.

Sidenote: std::ranges::dangling is neat :)

This matches the behavior of std::span, as Span satisfies the same concepts.
@laanwj
Copy link
Member

laanwj commented Oct 8, 2021

So the idea, as I understand, is that this adds compile-time checking capabilities a bit like rust? Yes that's really neat.

@theuni
Copy link
Member Author

theuni commented Oct 12, 2021

@laanwj Yes, but in this case we're actually opting out of some of those protections because of the semantics of Span. By default, the returned iterator type for a potentially dangling reference is a std::ranges::dangling. By opting in to enable_borrowed_range, we get a real return type. Otherwise it'd fail to compile the *iter dereference in the printf. This is also used by std::string_view, for example.

The enable_view seems to be another way of saying pretty much the same thing but for a different semantic reason.

I found it useful to copy/paste a little cheat-sheet of these concepts from libstdc++'s implementation:

/// [range.range] The range concept.
template<typename _Tp>
  concept range = requires(_Tp& __t)
    {
  ranges::begin(__t);
  ranges::end(__t);
    };

template<typename _Tp>
  concept view
    = range<_Tp> && movable<_Tp> && default_initializable<_Tp>
  && enable_view<_Tp>;

// Part of the constraints of ranges::borrowed_range
template<typename _Tp>
  concept __maybe_borrowed_range
  = is_lvalue_reference_v<_Tp>
    || enable_borrowed_range<remove_cvref_t<_Tp>>;

template<typename _Tp>
  concept borrowed_range
    = range<_Tp> && __detail::__maybe_borrowed_range<_Tp>;

/// A range which can be safely converted to a view.
template<typename _Tp>
  concept viewable_range = range<_Tp>
    && (borrowed_range<_Tp> || view<remove_cvref_t<_Tp>>);

In the above example, enabling enable_borrowed_range alone is enough to make both examples compile, because (I believe) the first only requires a viewable_range.

Interestingly, libstdc++'s definitions line up with the c++20 final draft spec, but differ from cppreference. I'm assuming that the latter is stale, so it's probably worth using the draft spec for reference for now.
I actually had this backwards. cppreference incorporates LWG 3481, which libstdc++ includes as of last week and P2415R2, which has not yet been integrated.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 1, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23413 (Replace MakeSpan helper with Span deduction guide by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 3, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

1 similar comment
@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@maflcko
Copy link
Member

maflcko commented Jul 25, 2022

Closing for now. Let us know when you are working on this again and if it should be reopened.

@maflcko maflcko closed this Jul 25, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants