3

I noticed that I can seemingly get the same logic(copy elements matching some predicate into vector) both using ranges::copy_if and also using vector constructor taking 2 iterators(by providing it with filter_view .begin() and .end()).

#include <algorithm>
#include <iostream>
#include <ranges>
#include <vector>
#include <utility>
#include <fmt/ranges.h>

const std::vector<int> vals{1,2,3,47,8472};
const auto filter_pred = [](const int i){return i%2==0;};

void fna(){
    std::vector<int> result;
    std::ranges::copy_if(vals, std::back_inserter(result), filter_pred);
    std::cout << fmt::format("{}", result) << std::endl;
}

void fn1(){
    auto filtered = vals | std::views::filter(filter_pred);
    std::vector<int> result {filtered.begin(), filtered.end()};
    std::cout << fmt::format("{}", result) << std::endl;
}

std::vector<int> fna_val(){
    std::vector<int> result;
    std::ranges::copy_if(vals, std::back_inserter(result), filter_pred);
    return result;
}

std::vector<int> fn1_val(){
    auto filtered = vals | std::views::filter(filter_pred);
    return {filtered.begin(), filtered.end()};
}

int main(){
    fna();
    fn1();
    std::cout << fmt::format("{}", fna_val()) << std::endl;
    std::cout << fmt::format("{}", fn1_val()) << std::endl;
}

Is there a reason to prefer one over other? What I can think of:

  • it is unlikely that either is more efficient, since I doubt that any real optimizations can be done since because _if in copy_if means destination size is unknown.
  • compile speed: copy_if could be faster to compile since it does not require <ranges> header, but did not benchmark
  • const all the things: copy_if requires mutable destination container, range construction does not
  • verbosity: when return type of function is known we do not need to spell out the std::vector in body, {} works. Would be nice if vector understood ranges so we would not have to use .begin() .end(), but it does not.
3
  • @康桓瑋 godbolt.org/z/K51P8vPMz Maybe I messed up something when c/p but code seems to compile. Commented Oct 11, 2021 at 11:35
  • What I mean is that when vals is views::iota(0) | views::take(5), fn1() will not work because the vector must accept the iterator-pairs of same types. Commented Oct 11, 2021 at 11:40
  • @康桓瑋 ah true, it does not work for ranges that are not en.cppreference.com/w/cpp/ranges/common_range... great point. Commented Oct 11, 2021 at 12:32

2 Answers 2

2

Just re-copying our comparison:

std::vector<int> fna_val(){
    std::vector<int> result;
    std::ranges::copy_if(vals, std::back_inserter(result), filter_pred);
    return result;
}

std::vector<int> fn1_val(){
    auto filtered = vals | std::views::filter(filter_pred);
    return {filtered.begin(), filtered.end()};
}

First things first. Never write this:

    return {filtered.begin(), filtered.end()};

For types where brace-initialization does something special (like vector!), it is simply asking for trouble to use brace-initialization in a context where you actively do not want the special brace-initialization behavior. Because:

vector<int> v = {filtered.begin(), filtered.end()};
vector      w = {filtered.begin(), filtered.end()};

This does what you want for v, but w is a vector of two filter_view<...>::iterators. That's pretty subtle. So I would strongly encourage avoiding ever doing this — because it is so easy to get wrong.


That said, if we consider how the vector initialization actually works if you spell it correctly:

  • The ranges::copy_if version is basically doing push_back in a loop. That means that you will do some number of allocations, roughly the log2 of the number of elements you're pushing back (depends on the growth factor). So for 100 elements, that could be 7 allocations.
  • The views::filter version will do one single allocation, which it will figure out by walking the entire range twice — once to count how many elements are in it, and then once to actually read them.

Which is better? It depends on many factors — how many elements you have, how expensive the filter predicate is (the more expensive it is, the worse it would be to invoke it twice per element), and how high the percentage of elements that satisfy the predicate is. For the latter, consider that the copy_if version could start by simply doing:

result.reserve(vals.size());

That ensures that we have exactly 1 allocation. And if like 70% of our elements satisfy filter_pred, that's probably a good trade-off. If like 1% of our elements satisfy filter_pred, that allocation is going to be much bigger than it should be. Maybe that's bad.

So... it depends. But please don't use brace-initialization unless you actually want brace-initialization.


Note that in C++23, the filter version would be better written like

return vals
     | std::views::filter(filter_pred)
     | std::ranges::to<std::vector>(); 

Which additionally avoids the question of how to specifically spell that initialization.

Sign up to request clarification or add additional context in comments.

1 Comment

Thank you for the answer. I think {} init is fine for tiny functions where return type is visible... But yes I have bugged my code few times, in longer function creating local variables, e..g. getting set of 2 elements(iterators) and then thinking count of unique elements is 2. As for ::to - this question is old, so I did not have C++23 ::to then. Also I wonder if en.cppreference.com/w/cpp/ranges/approximately_sized_range.html is useful here in your discussion of realloc size(e.g. if I know or strongly believe 33% of elements remain).
0

You should always measure the attribute which is important for you, if it is speed than that, if it is binary size, or compile time, than those. For my case, binary size was important, and I found significant differences.

For example, I just wanted to copy a range of bytes (can be string_view or array of std::byte) to an std::array<char>. I started with this:

    const auto printableChars = std::views::take(SIZE)
        | std::views::transform([](auto b) { return static_cast<char>(b); })
        | std::views::filter([](char c) { return c >= ' ' && c <= '~'; })
        | std::views::take_while([](char c) { return c != '\0'; });
    std::ranges::copy(range | printableChars, out.begin());

The procedural approach was half the size:

    for (size_t i = 0; auto b : range) {
        const char c = static_cast<char>(b);
        if (c == '\0') { break; }
        if (c < ' ' || c > '~') { continue; }
        out[i] = c;
        if (++i >= SIZE) { break; }
    }

But using copy_if was even smaller:

    const auto view = std::views::take(99)
        | std::views::transform([](auto b) { return static_cast<char>(b); })
        | std::views::take_while([](char c) { return c != '\0'; });
    constexpr auto isPrintable = [](auto c){ return c >= ' ' && c <= '~'; };
    std::ranges::copy_if(range | view, out.begin(), isPrintable);

I tried ordering the views, sometimes it does help, or I tried ranges::copy_n instead of views::take. Of course the whole thing depends on compiler and version, so all in all, you have to measure your case.

Maybe I would try std::ranges::to<std::vector>, might be better or worse.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.