Skip to main content
Suggest passing a range instead of separate iterators
Source Link
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

One alternative to separate "destructive" and "copying" functions that I've used successfully in the past is to accept the collection by value. Then a caller can use std::move() to avoid copying elements when they do not need to be preserved.

Modified code

CorrectedOne alternative to separate "destructive" and simplified"copying" functions that I've used successfully in the past is to accept the collection by value. Then a caller can use std::move() to avoid copying elements when they do not need to be preserved.

That might look like this:

#include <algorithm>
#include <concepts>
#include <functional>
#include <vector><ranges>

template<classtemplate<std::ranges::bidirectional_range IteratorContainer, 
 class Compare>
typename Iterator      std::value_typepredicate<std::ranges::range_value_t<Container>, stone_smash_destructivestd::ranges::range_value_t<Container>> Compare>
auto stone_smash(IteratorContainer firstvalues, IteratorCompare last,comp = std::less<>{})
{
    auto const first = values.begin();
    auto last = values.end();
      
    return std::ranges::range_value_t<Container>{};
}

And we'd call it slightly differently:

    assert(expected == stone_smash(v, comp));
    assert(expected == stone_smash(std::move(v), comp));

Or we can call it destructively from an iterator pair by constructing a std::range::subrange from them.


Modified code

Corrected and simplified:

#include <algorithm>
#include <concepts>
#include <functional>
#include <ranges>

template<std::ranges::bidirectional_range Container,
         std::predicate<std::ranges::range_value_t<Container>, std::ranges::range_value_t<Container>> Compare>
auto stone_smash(Container values, Compare comp = std::less<>{})
{
    auto const first = values.begin();
    auto last = values.end();

    std::make_heap(first, last, comp);
    while (first < last)
    {
        std::pop_heap(first, last--, comp);
        if (first == last) {
            return *first;
        }

        auto const y = *last;
        std::pop_heap(first, last--, comp);
        auto const x = *last;
        if (comp(x, y)) {
            *last = y - x;
            std::push_heap(first, ++last, comp);
        }
    }
    return {0};
}

template<class Iterator, class Compare>
typename Iteratorstd::value_type stone_smash(Iterator first, Iterator last,
                                          Compare comp = stdranges::less<>range_value_t<Container>{})
{
    auto v = std::vector(first, last);
    return stone_smash_destructive(v.begin(), v.end(), comp);
}

One alternative to separate "destructive" and "copying" functions that I've used successfully in the past is to accept the collection by value. Then a caller can use std::move() to avoid copying elements when they do not need to be preserved.

Modified code

Corrected and simplified:

#include <algorithm>
#include <functional>
#include <vector>

template<class Iterator, class Compare>
typename Iterator::value_type stone_smash_destructive(Iterator first, Iterator last,
                                                      Compare comp = std::less<>{})
{
    std::make_heap(first, last, comp);
    while (first < last)
    {
        std::pop_heap(first, last--, comp);
        if (first == last) {
            return *first;
        }

        auto const y = *last;
        std::pop_heap(first, last--, comp);
        auto const x = *last;
        if (comp(x, y)) {
            *last = y - x;
            std::push_heap(first, ++last, comp);
        }
    }
    return {0};
}

template<class Iterator, class Compare>
typename Iterator::value_type stone_smash(Iterator first, Iterator last,
                                          Compare comp = std::less<>{})
{
    auto v = std::vector(first, last);
    return stone_smash_destructive(v.begin(), v.end(), comp);
}

One alternative to separate "destructive" and "copying" functions that I've used successfully in the past is to accept the collection by value. Then a caller can use std::move() to avoid copying elements when they do not need to be preserved.

That might look like this:

#include <algorithm>
#include <concepts>
#include <functional>
#include <ranges>

template<std::ranges::bidirectional_range Container, 
         std::predicate<std::ranges::range_value_t<Container>, std::ranges::range_value_t<Container>> Compare>
auto stone_smash(Container values, Compare comp = std::less<>{})
{
    auto const first = values.begin();
    auto last = values.end();
      
    return std::ranges::range_value_t<Container>{};
}

And we'd call it slightly differently:

    assert(expected == stone_smash(v, comp));
    assert(expected == stone_smash(std::move(v), comp));

Or we can call it destructively from an iterator pair by constructing a std::range::subrange from them.


Modified code

Corrected and simplified:

#include <algorithm>
#include <concepts>
#include <functional>
#include <ranges>

template<std::ranges::bidirectional_range Container,
         std::predicate<std::ranges::range_value_t<Container>, std::ranges::range_value_t<Container>> Compare>
auto stone_smash(Container values, Compare comp = std::less<>{})
{
    auto const first = values.begin();
    auto last = values.end();

    std::make_heap(first, last, comp);
    while (first < last)
    {
        std::pop_heap(first, last--, comp);
        if (first == last) {
            return *first;
        }

        auto const y = *last;
        std::pop_heap(first, last--, comp);
        auto const x = *last;
        if (comp(x, y)) {
            *last = y - x;
            std::push_heap(first, ++last, comp);
        }
    }
    return std::ranges::range_value_t<Container>{};
}
By-value interface
Source Link
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

One alternative to separate "destructive" and "copying" functions that I've used successfully in the past is to accept the collection by value. Then a caller can use std::move() to avoid copying elements when they do not need to be preserved.


The logic is clear and precise. I might suggest turning around the test if … else return to test whether it can return:

The logic is clear and precise. I might suggest turning around the test if … else return to test whether it can return:

One alternative to separate "destructive" and "copying" functions that I've used successfully in the past is to accept the collection by value. Then a caller can use std::move() to avoid copying elements when they do not need to be preserved.


The logic is clear and precise. I might suggest turning around the test if … else return to test whether it can return:

Source Link
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

I confess I was initially surprised to see the low-level heap functions rather than std::priority_queue<>. But this makes sense, as it avoids having to copy the elements into the queue's container. It's good that we mention this destructiveness in the name!

The logic is clear and precise. I might suggest turning around the test if … else return to test whether it can return:

        std::pop_heap(first, last--, comp);
        if (first == last) {
            // single element - return it
            return *first;
        }

We can also provide extra clarity that we don't modify x and y by declaring them auto const:

        auto const y = *last;
        std::pop_heap(first, last--, comp);
        auto const x = *last;
        if (comp(x, y)) {
            *last = y - x;
            std::push_heap(first, ++last, comp);
        }

We don't need separate overloads for defaulting the comparator - just provide a default argument:

#include <algorithm>
#include <functional>

template<class Iterator, class Compare>
typename Iterator::value_type stone_smash_destructive(Iterator first, Iterator last,
                                                      Compare comp = std::less<>{})
template<class Iterator, class Compare>
typename Iterator::value_type stone_smash(Iterator first, Iterator last,
                                          Compare comp = std::less<>{})

I don't like make_vector's use of explicit constructor { } - we normally use ( ) for non-initialiser-list construction:

template<class Iterator>
auto make_vector(Iterator first, Iterator last)
{
    return std::vector<typename Iterator::value_type>(first, last);
}

With current standard library's deduction guides, we can omit the template argument:

template<class Iterator>
auto make_vector(Iterator first, Iterator last)
{
    return std::vector(first, last);
}

At this point, it doesn't merit being a function - it's clearer just inlined where we need it.


The constructor of binary_condition_counter accepts a forwarding reference, but then copies into condition where it should std::forward<>():

    binary_condition_counter(std::size_t& count, BinaryCondition&& condition)
        : count(count),
          condition(std::forward<BinaryCondition>(condition))
    {}

However, we don't need to write this constructor, as it's exactly the same as the aggregate initialiser.

We might as well make condition const, since the class is already unassignable due to the reference member count.

We declare our result_type but then return bool from operator(). I would return auto or result_type, even if we expect the wrapped comparator to return bool.


make_binary_condition_counter() also accepts a forwarding reference, but this time chooses std::move() instead of std::forward<>(). I would simply remove this function, as type deduction works perfectly fine for the constructor.


Modified code

Corrected and simplified:

#include <algorithm>
#include <functional>
#include <vector>

template<class Iterator, class Compare>
typename Iterator::value_type stone_smash_destructive(Iterator first, Iterator last,
                                                      Compare comp = std::less<>{})
{
    std::make_heap(first, last, comp);
    while (first < last)
    {
        std::pop_heap(first, last--, comp);
        if (first == last) {
            return *first;
        }

        auto const y = *last;
        std::pop_heap(first, last--, comp);
        auto const x = *last;
        if (comp(x, y)) {
            *last = y - x;
            std::push_heap(first, ++last, comp);
        }
    }
    return {0};
}

template<class Iterator, class Compare>
typename Iterator::value_type stone_smash(Iterator first, Iterator last,
                                          Compare comp = std::less<>{})
{
    auto v = std::vector(first, last);
    return stone_smash_destructive(v.begin(), v.end(), comp);
}
#include <cstddef>

template<class BinaryCondition>
struct binary_condition_counter
{
    std::size_t& count;
    BinaryCondition const condition;

    using result_type = typename BinaryCondition::result_type;
    using first_argument_type = typename BinaryCondition::first_argument_type;
    using second_argument_type = typename BinaryCondition::second_argument_type;

    auto operator()(first_argument_type a, second_argument_type b) const
    {
        ++count;
        return condition(a, b);
    }
};