3

I'm trying to use a range based iterator with a set of unique_ptr instances but I'm getting the following compilation error:

C2280: 'std::unique_ptr<Component,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function

The basics of the code is below:

#include <set>
#include <memory>

std::set<std::unique_ptr<Component>>* m_components;

class Component
{
    void DoSomething(){};
};

void ProcessComponents()
{
    for (auto componentsIterator : *m_components)

    {
        componentsIterator->DoSomething();
        componentsIterator++;
    }
}

Any idea why this would be a problem or how to solve it?

tod
  • 1,539
  • 4
  • 17
  • 43
jhegedus
  • 61
  • 2
  • 8
  • `std::set>*` - in all likelihood, the `set` itself doesn't need to be a pointer. Also, it's odd that you use a smart pointer to hold the elements within the `set` but decide to manage the `set`'s memory manually. – Praetorian May 10 '14 at 17:21
  • @Praetorian, the set is actually a member of a class that is initialized when the class is created and destroyed in the destructor. I tried to create a simple example to illustrate my question. It seemed like the right way but I'm a bit of a beginner so let me know if there is a better way. – jhegedus May 10 '14 at 17:27
  • 2
    `std::set> m_components` is the better way. It'll be instantiated and destroyed along with your class instances, and there's no need to `new` and `delete` it any more. – Praetorian May 10 '14 at 17:29
  • 2
    awesome! thanks for the tip. My first time posting here. Just created an account and am already amazed at how responsive and how valuable a resource it is. – jhegedus May 10 '14 at 17:33

1 Answers1

9
for (auto componentsIterator : *m_components)

That auto expands to std::unique_ptr<Component>, which means that you are trying to take a copy of each element. IOW, that loop actually is:

for(auto it=m_components->begin(); it!=m_components->end(); ++it)
{
    std::unique_ptr<Component> componentsIterator=*it;
    componentsIterator->DoSomething();
    componentsIterator++;
}

As you can see, you are invoking std::unique_ptr<Component> copy constructor, but the copy constructor of unique_ptr is deleted (because it is against the unique_ptr semantic).

Use auto & to take a reference instead.

(BTW, componentsIterator there is not a sensible name, since it's not an iterator, it's the actual element)

Matteo Italia
  • 123,740
  • 17
  • 206
  • 299
  • thanks for the instant response. it helps but doesn't quite get me there as I still get an error on the increment operator C2676: binary '++' : 'const std::unique_ptr>' does not define this operator or a conversion to a type acceptable to the predefined operator – jhegedus May 10 '14 at 17:15
  • @jhegedus: what do you intend to do with that `++`? `unique_ptr` doesn't define any `++` operator, is it a thing of your `Component` class or you put it there to move to the next element in the loop? – Matteo Italia May 10 '14 at 17:17
  • I thought I needed the ++ to iterate through the loop. Did I misunderstand? – jhegedus May 10 '14 at 17:19
  • @jhegedus: yes, you misunderstood; the range-based `for` already takes care of that. – Matteo Italia May 10 '14 at 17:19
  • actually, I see I misunderstood, that is in the for you expanded out for me. Thanks again – jhegedus May 10 '14 at 17:20
  • @jhegedus: in general, remember that with the range-based `for` you don't need to care about iterators at all - it will just loop automatically over all the *elements* of the container, without you having to do anything special. – Matteo Italia May 10 '14 at 17:22