8

TL;DR: I'm playing around with ranges and the corresponding range adaptors from the Ranges library. Both range adaptors std::views::take_while and std::views::filter take a predicate to exclude certain elements from the input sequence. Why does take_while take a const predicate while filter does not?

Background story

I have an std::vector<int> and want to iterate over it, but I want to stop iterating when hitting a 5. By using the range adaptor std::views::take_while I can implement that as follows:

std::vector<int> v { 8, 2, 5, 6 };

for (int i : v | std::views::take_while([](int i) { return i != 5; })) {
    std::cout << "Value: " << i << std::endl;
}

Output:

Value: 8
Value: 2

However, I now want to process the 5 as well, so the loop must run one iteration step further. I didn't find a suitable range adaptor, so I wrote the following stateful lambda expression:

auto cond = [b = true](int i) mutable {
    return b ? b = (i != 5), true : false;
};

This lambda expression remembers when the condition i != 5 is violated and returns false on the next call. I then passed it to the std::views::take_while as follows:

for (int i : v | std::views::take_while(cond)) {
    std::cout << "Value: " << i << std::endl;
}

However, for the above code, the compiler throws a long error message. Since I couldn't spot the problem, I closely inspected the declaration of std::views::take_while and found that the predicate Pred must be const. Looking for an alternative, I checked the declaration of std::views::filter. Interestingly, Pred is not required to be const here. So I passed the above mutable lambda to the range adaptor std::views::filter as follows:

for (int i : v | std::views::filter(cond)) {
    std::cout << "Value: " << i << std::endl;
}

This code compiles and gives the desired output:

Value: 8
Value: 2
Value: 5

Code on Wandbox

This leads me to my question: Why does std::views::take_while a const predicate, while std::views::filter does not?

honk
  • 9,137
  • 11
  • 75
  • 83
  • 3
    Could be because `filter` does not model `random_access_range` whereas `take_while` does. I imagine this may require calling the predicate out of order, which is likely to throw off a stateful predicate. – Igor Tandetnik Apr 09 '21 at 15:38
  • 1
    @IgorTandetnik: Good point; at least that sounds sensible for me. However, I cannot imagine a use case where the predicate would be called out of order. If anybody has an idea in which situation that could happen, please let us know :) – honk Apr 09 '21 at 15:46
  • @IgorTandetnik That's not it, no. See my answer. – Barry Apr 09 '21 at 17:06

2 Answers2

4

Why this is a bad idea

Let's produce a version that compiles and see what it actually does:

struct MutablePredicate {
    mutable bool flag = true;

    auto operator()(int i) const -> bool {
        if (flag) {
            flag = (i != 5);
            return true;
        } else {
            return false;
        }
    }
};

std::vector<int> v = {8, 2, 5, 6};
auto r = v | std::views::take_while(MutablePredicate{});

fmt::print("First: {}\n", r);
fmt::print("Second: {}\n", r);

This prints {8, 2, 5} the first time, as desired. And then {} the second time. Because of course, we modified the predicate and so we get different behavior entirely. This completely breaks the semantics of this range (because your predicate fails to be equality-preserving), and all sorts of operations just totally fail as a result.

The resulting take_view is a random-access range. But just think about what happens when you use iterators into it:

std::vector<int> v = {8, 2, 5, 6};
auto r = v | std::views::take_while(MutablePredicate{});

auto it = r.begin();
it += 2;                // this is the 5
assert(it != r.end());  // does not fire, because we're not at the end
assert(it == r.end());  // does not fire, because we're at the end??

This is all sorts of weird and makes reasoning about this impossible.

Why the difference in constraints

The range adaptors in C++20 try to minimize the number of template instantiations by optimizing around "simple-view": V is a simple-view if both V and V const are ranges with the same iterator/sentinel types. For those cases, adaptors don't provide both begin() and begin() const... they just provide the latter (since there's no difference in these cases, and begin() const always works, so we just do it).

Our case is a simple-view, because ref_view<vector<int>> only provides begin() const. Whether we iterate that type as const or not, we still get vector<int>::iterators out of it.

As a result, take_while_view in order to support begin() const needs to require that Pred const is a unary predicate, not just Pred. Since Pred has to be equality-preserving anyway, it's simpler to just require that Pred const is a unary predicate rather than potentially supporting begin() /* non-const */ if only Pred but not Pred const is a unary predicate. That's just not an interesting case worth supporting.

filter_view is not const-iterable, so it doesn't have to this consideration. It's only ever used as non-const, so there's no Pred const that it ever meaningfully has to consider as being a predicate.

What you should do instead

So if you don't actually need lazy evaluation, we can eagerly calculate the end iterator:

auto e = std::ranges::find_if(v, [](int i){ return i == 5; });
if (e != v.end()) {
    ++e;
}
auto r = std::ranges::subrange(v.begin(), e);
// use r somehow

But if you do need lazy evaluation, one way to do this is create your own adaptor. For a bidirectional+ range, we can define a sentinel such that we match the iterator if either (a) it's at the end of the underlying view's base or (b) it's not at the beginning of the range and the previous iterator matches the underlying view's end.

Something like this (will only work on views that have a .base() since it only makes sense to and_one an adapted range):

template <std::ranges::bidirectional_range V>
    requires std::ranges::view<V>
class and_one_view {
    V base_ = V();
    using B = decltype(base_.base());

    class sentinel {
        friend and_one_view;
        V* parent_ = nullptr;
        std::ranges::sentinel_t<V> end_;
        std::ranges::sentinel_t<B> base_end_;

        sentinel(V* p)
            : parent_(p)
            , end_(std::ranges::end(*parent_))
            , base_end_(std::ranges::end(parent_->base()))
        { }
    public:
        sentinel() = default;
        auto operator==(std::ranges::iterator_t<V> it) const -> bool {
            return it == base_end_ ||
                it != std::ranges::begin(*parent_) && std::ranges::prev(it) == end_;
        }
    };
public:
    and_one_view() = default;
    and_one_view(V b) : base_(std::move(b)) { }

    auto begin() -> std::ranges::iterator_t<V> { return std::ranges::begin(base_); }
    auto end() -> sentinel { return sentinel(&base_); }
};

which for the purposes of demonstration we can make pipeable with libstdc++'s internals:

struct AndOne : std::views::__adaptor::_RangeAdaptorClosure
{
    template <std::ranges::viewable_range R>
        requires std::ranges::bidirectional_range<R>
    constexpr auto operator()(R&& r) const {
        return and_one_view<std::views::all_t<R>>(std::forward<R>(r));
    }
};
inline constexpr AndOne and_one;

And now, because we adhere to all the semantic constraints of all the library components, we can just use the adapted range as a range:

std::vector<int> v = {8, 2, 5, 6};
auto r = v | std::views::take_while([](int i){ return i != 5; })
           | and_one;

fmt::print("First: {}\n", r);   // prints {8, 2, 5}
fmt::print("Second: {}\n", r);  // prints {8, 2, 5} as well

Demo.

Barry
  • 286,269
  • 29
  • 621
  • 977
  • Thanks a lot! I had no idea what's under the hood of C++20 yet. You made me read a bit about concepts and sentinels and I think I now roughly understand your answer. One thing: I guess by "_`simple-view`_" you mean [this](https://en.cppreference.com/w/cpp/ranges/view), right? I can see from the declaration that it must be a [range](https://en.cppreference.com/w/cpp/ranges/range), but from where can I see that `V` and `V const` must have the same iterator/sentinel types? – honk Apr 11 '21 at 18:56
  • Barry does ranges have some equivalent of std::next, something that would give op the view that also includes the 5? I know it is tricky with ranges since it is unclear if you want window shifted or expanded at the end... – NoSenseEtAl Apr 12 '21 at 13:57
  • 1
    @NoSenseEtAl If you want to use `next`, sure the code I wrote with `if (e != v.end()) ++e;` could also be `e = ranges::next(e, 1, v.end());`? – Barry Apr 12 '21 at 14:32
  • @Barry Yes, that Is what I meant. I was hoping for something shorter, but I guess ranges did not want to pay overhead of knowing underlying range .end()... :/ – NoSenseEtAl Apr 12 '21 at 18:38
  • "*filter_view is not const-iterable, so it doesn't have to this consideration. It's only ever used as non-const, so there's no Pred const that it ever meaningfully has to consider as being a predicate.*" But `drop_while_view` is also not `const`-iterable, but requires `const Pred`, I mean, isn't that inconsistent? – 康桓瑋 Jul 21 '22 at 15:11
1

A std::indirect_unary_predicate is required to be a std::predicate, which is required to be a std::regular_invocable. And that requires:

The invoke function call expression shall be equality-preserving ([concepts.equality]) and shall not modify the function object or the arguments.

Your lambda is not "equality-preserving", since the same inputs can give rise to different outputs, and it definitely modifies the function object. Since your type does not satisfy the non-syntactic conditions of the concept, your code has UB.

So really, it doesn't matter whether one is const and the other is not; the concepts effectively require them to behave as if they're const.

Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
  • Just to confirm if I got that right: You mean that passing my mutable lambda to `std::views::filter` causes UB? If so, I wonder why I'm not prevented from doing so. At least `std::views::take_while` does ;) – honk Apr 09 '21 at 16:12
  • @honk: Using `const Pred` is no guarantee that the predicate is equality-preserving. That's why `std::regular_invocable` doesn't require the type it is given to be `const` or something. So it doesn't offer any greater assurance of anything. – Nicol Bolas Apr 09 '21 at 16:13
  • I think I'm beginning to understand. I will try to be careful and not mess around with the predicates too much. Thank you for preventing me from a lot of future mistakes! ;) – honk Apr 09 '21 at 16:22