4

I have this code that fails (Explorer):

#include <range/v3/view/any_view.hpp>
#include <vector>

int main() {
   std::vector<int> a{1};

   const ranges::any_view<int, ranges::category::forward> av(a);
   ranges::begin(av);
}

And was in the impression that begin returns a new iterator into the range, that allows me to iterate through it several times. Why would it need a modifiable range for that?

As reference (but I'm more interested in the reason of the above than (just) a solution to my problem below), in my real code, this appears as

void merge(ranges::any_view<int, ranges::category::forward> av) {
   for(auto x : av) { ... }
}

And unfortunately clang-tidy warns that av should be made const& because it's passed by value but not moved inside of the function body.


My suspicion is that it has to do with the "amortized constant time complexity" requirement of begin / end calls. The following fails because I think the begin iterator can't be cached in the const view returned by drop:

std::list<int> b{1, 2, 3, 4};

const auto latter = ranges::views::all(b) | ranges::views::drop(2);
ranges::begin(latter);

So just in case the type-erased variable contains such a view, it has to cache the first result of begin for all wrapped types. However, even if I change av to random_access, it won't let me call begin on a const any_view:

const ranges::any_view<int, ranges::category::random_access> av(a);

And if this is indeed the reason why it fails for input, I wonder whether there's a way around this? Like the std::move_only_function allows a moving std::function (kind of) in C++23.

Barry
  • 286,269
  • 29
  • 621
  • 977
Johannes Schaub - litb
  • 496,577
  • 130
  • 894
  • 1,212
  • Add `const` for value types instead of `any_view` eg. `ranges::any_view`? – 康桓瑋 Aug 11 '23 at 12:33
  • I'm not sure there's any good reason for this. `any_view` is [implemented](https://github.com/ericniebler/range-v3/blob/9aa41d6b8ded2cf5e8007e66a0efd1ab33dbf9a5/include/range/v3/view/any_view.hpp#L600-L609) as holding a `unique_ptr` to an interface, which `begin_cursor()` just dereferences. So just... marking `begin_cursor()` and `end_cursor` `const` just works. – Barry Aug 11 '23 at 13:43
  • PRs welcome :-) – Barry Aug 11 '23 at 13:43

1 Answers1

2

First, I'd call this very much a clang-tidy defect. It shouldn't complain about your passing any_view<T> by value; nor string_view; nor span<T>; nor function_ref<Sig>; nor any other type-erased view-semantic type. The entire point of these types is that you pass them by value.

(Likewise, I would hope that clang-tidy doesn't complain about passing unique_ptr<T> or shared_ptr<T> by value. In those cases, the reason to pass the type at all is to participate in lifetime, and if you take by const& you're failing to participate in lifetime, which can cause awful bugs in a multithreaded program. Vice versa, if you don't need to participate in lifetime you should take const T& or const T*, not const unique_ptr<T>&.)

Okay, so, @Barry says "PRs welcome [on any_view, I'm going to assume]." I'm not 100% sure, but I think that would be a bad idea. Iterating a view in C++20-Ranges-world is a mutating operation by definition. any_view type-erases over that operation. Thus iterating an any_view is a mutating operation by definition. Sure, one could physically slap a const qualifier on any_view::begin, but that would be a semantic mistake, just like it was when C++11 did it for std::function::operator().

Here's what std::function does wrong:

auto lam = [i=0]() mutable { return ++i; };
std::function<int()> f = std::move(lam);
auto worker = [](const std::function<int()>& f) {
  f();  // OK, const operation is presumed thread-safe
};
auto t1 = std::jthread(worker, std::cref(f));
auto t2 = std::jthread(worker, std::cref(f));
  // and boom goes the dynamite

Both t1 and t2 will try to modify f's controlled lambda's capture i at the same time. This is UB.

Now here's how any_view would go wrong if it were const-iterable:

auto r = std::views::filter(std::views::iota(1), isOdd);
std::any_view<int> v = std::move(r);
auto worker = [](const std::any_view<int>& v) {
  v.begin();  // OK, const operation is presumed thread-safe
};
auto t1 = std::jthread(worker, std::cref(v));
auto t2 = std::jthread(worker, std::cref(v));
  // and boom goes the dynamite

Both t1 and t2 will try to modify v's controlled filter_view's cache at the same time. This is UB.


std::move_only_function and std::function_ref (and soon std::copyable_function IIUC) deal with this problem by providing different signatures for callability versus const-callability.

auto lam = [i=0]() mutable { return ++i; };
std::copyable_function<int()> f = std::move(lam);  // OK
auto worker = [](const std::copyable_function<int()>& f) {
  f();  // error, can't use the mutable operator() on a const f
};

auto lam = [i=0]() mutable { return ++i; };
std::copyable_function<int() const> f = std::move(lam);
  // error, can't store a mutable lam in a copyable_function<int() const>
auto worker = [](const std::copyable_function<int() const>& f) {
  f();  // OK
};

There's no natural place to hang the const on any_view's template parameter; but I could imagine a library providing both lib::any_view<T> and lib::const_any_view<T>, if there were really any demand for the latter.

As I said first of all, though, I don't think there should be any demand for the latter. The only reason you were trying to do const any_view& at all is because clang-tidy told you to; and that's clearly just a bug in clang-tidy.

Quuxplusone
  • 23,928
  • 8
  • 94
  • 159
  • 1
    Thanks, this is very enlightening. IIRC if a range is borrowable, its state is stored in the iterators, right? Are those the ones that are const-iterable? – Johannes Schaub - litb Aug 11 '23 at 20:18
  • Because any_view is eypensivezto copy, like boost::any, I wanted to avoid copies. I sort of disagree with "the entire point of them is to pass them by value". Their point is to be non-owning, and of some of them to pass them by value. But the byvalue passing is not an intrinsic property of non-ownership. It is because some of them are cheap to copy. In fact I would agree the non-ownering makes it plausible to use passbyref where appropriate, _because_ keeping a separate lifetime-extender is not a concern there. – Johannes Schaub - litb Aug 12 '23 at 09:47
  • I also have another question! Why is begin's caching considered a modification even though it doesn't change the result? Is it not more appropriate to make the cache member mutable and the begin function const? – Johannes Schaub - litb Aug 12 '23 at 11:50
  • NVM I came to understand that the std views are a huge mess and I certainly won't use them and likely neither ranges-v3. Belleviews looks good but I'm not sure whether it will survive. – Johannes Schaub - litb Aug 12 '23 at 12:55
  • "if a range is borrowable, its state is stored in the iterators" — Yes, a borrowed range is "boneless" — "deboning" it into an iterator/sentinel pair is totally safe. So any state it has, must be stored in the iterator/sentinel. "Are those the ones that are const-iterable" — As usual in C++, you're philosophically correct but technically there are more wrinkles. Suppose the iterator type is non-copyable; then you might not be able to extract it with `begin() const`. E.g. `subrange` is `borrowed_range` but still not const-iterable. https://godbolt.org/z/r9T6xsjGj – Quuxplusone Aug 13 '23 at 14:12