current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

For setters that set a member collection, I write template functions so as to accept any collection type as input:

class HasItems
{
public:
    template <typename C>
    void items(const C& items) {
        // Copy-and-swap omitted
        m_items.assign(std::begin(items), std::end(items));
    }

private:
    std::vector<Item> m_items;
}

But suppose Item is big, and I want to move them around instead of copy if possible. And furthermore, suppose someone passed me an xvalue std::vector<Item>; I could just move that whole container into place and avoid touching the elements at all! That looks like this:

class HasItems
{
public:
    template <typename C>
    void items(C&& items) {
        // Helper function implemented below
        assign(m_items, std::forward<C>(items));
    }

private:
    std::vector<Item> m_items;
}

// Assigns the contents of src to dest, moving as much as possible
template <typename C1, typename C2>
void assign(C1& dest, C2&& src) {
    assign(dest, std::forward<C2>(src), std::is_assignable<C1&, C2&&>{});
}

// Called if we can use operator=
template <typename C1, typename C2>
void assign(C1& dest, C2&& src, std::true_type use_equals) {
    dest = std::forward<C2>(src);
}

// Called if we must use dest.assign()
template <typename C1, typename C2>
void assign(C1& dest, C2&& src, std::false_type use_equals) {
    assign_elements(dest, std::forward<C2>(src), std::is_rvalue_reference<C2&&>{});
}

// Called if we can move the elements
template <typename C1, typename C2>
void assign_elements(C1& dest, C2&& src, std::true_type move) {
    dest.assign(std::make_move_iterator(std::begin(src)),
                std::make_move_iterator(std::end(src)));
}

// Called if we must copy the elements
template <typename C1, typename C2>
void assign_elements(C1& dest, C2&& src, std::false_type move) {
    dest.assign(std::begin(src), std::end(src));
}

Is this a good approach? In particular, is there a nice way to avoid the tag dispatch on std::is_rvalue_reference<C2&&> in the assign_elements() case? (It was avoided in the operator= case by using dest = std::forward<C2>(src) instead of using std::move() conditionally.) It seems to pass my simple set of unit tests.

share|improve this question

1 Answer 1

up vote 0 down vote

I've implemented this idiom and discovered a major caveat after a few weeks: it doesn't play nicely with boost::iterator_range or other collection views. An example:

std::vector<std::string> dest;
std::list<std::string> src{"foo", "bar"};
// Moves elements out of src, expected
assign(dest, std::move(src));

src = {"foo", "bar"};
// Also moves elements out of src!!!
assign(dest, boost::make_iterator_range(src));

This happens because boost::make_iterator_range(src) is an xvalue. I don't see a good way to distinguish between "container" (meaning safe to move from xvalues) and "container view" (meaning not safe to move from).

share|improve this answer
    
You should ask that on stackoverflow. My guess is you need assign to take iterators instead and do the right thing depending on if they are move iterators. –  nwp Jun 9 at 10:29

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.