4

I'm getting a lot of use of deleted function error. I just changed the pointer of weighted_pointer to unique_ptr. But I can't realize why I'm getting the error, any tip?

The likeatree is a DAG structure which can point to another struct or an element of stdDeque based on mask value.

The weight of weighted_pointer has mutable keyword because doesn't change where in the set will be.

#include <deque>
#include <set>
#include <vector>
#include <iostream>
#include <algorithm>
#include <memory>
#include <chrono>

using namespace std;

struct likeatree{
    unsigned int mask : 3;
    void * a;
    void * b;
};

struct weighted_pointer{
    mutable int weight;
    unique_ptr<likeatree> ptr;
};

struct ptrcomp{
    bool operator()(const weighted_pointer & lhs, const weighted_pointer & rhs) {
        if(lhs.ptr->mask < rhs.ptr->mask)
            return true;
        if(lhs.ptr->mask > rhs.ptr->mask)
            return false;
        if(lhs.ptr -> a < rhs.ptr->a)
            return true;
        if(lhs.ptr->a > rhs.ptr->a)
            return false;
        return lhs.ptr->b < rhs.ptr->b;
    }
};

vector<likeatree *> treeVector;
deque<bool> stdDeque(3);
vector<vector<bool>> boolMatrix{{0,0,0,0,0,0,0,0},{0,0,0,0,0,0,0,0},{0,0,0,0,0,0,0,0}};
set<weighted_pointer,ptrcomp> stdSet;

int main(){
    srand(time(NULL));
    likeatree * first_pointer = new likeatree{0,&input[0],nullptr};
    likeatree * second_pointer = first_pointer;
    unique_ptr<likeatree> tmp(first_pointer);
    weighted_pointer wp;
    wp.weight = 1;
    wp.pointer = move(tmp);
    stdSet.insert(move(wp));
    // I'd like to do it inline(or more but with variables that end of scope here), but this don't work. (And i don't keep a copy of the pointer)
    // stdSet.insert(move(weighted_pointer{1,move(make_unique<likeatree>(*new likeatree{0,&input[0],nullptr}))}));
    return 0;   
}

Edit: Changed code with a single case of the problem Edit: Solved. Missing a dereference when using make_unique.

Umuril Lyerood
  • 175
  • 1
  • 1
  • 9
  • You'll get better answers (and perhaps some insight into interpretation of errors) if you include the actual compiler error. Also useful is the compiler options you are using. I get no errors compiling this with `g++ -std=c++11 -c /tmp/31791982.cpp` (though I can't link successfully, due to missing `main()`). – Toby Speight Aug 03 '15 at 16:17
  • possible duplicate of [Copy constructor for a class with unique\_ptr](http://stackoverflow.com/questions/16030081/copy-constructor-for-a-class-with-unique-ptr) – ex-bart Aug 03 '15 at 16:18
  • @TobySpeight -std=c++11 -Wall. The main is at the end. – Umuril Lyerood Aug 03 '15 at 16:19
  • btw, you may write `bool ptrcomp::operator()(const weighted_pointer& lhs, const weighted_pointer& rhs) { return std::tie(lhs.ptr->mask, lhs.ptr->a, lhs.ptr->b) < std::tie(rhs.ptr->mask, rhs.ptr->a, rhs.ptr->b); }`. – Jarod42 Aug 03 '15 at 16:21
  • Sorry, I missed the scrollbar. I now get the errors, but can't improve on the answer (I've upvoted), except to say that if `mask` can be a plain `unsigned int` rather than a bitfield, you can simplify the comparator to `return std::tie(lhs.ptr->mask, lhs.ptr->a, lhs.ptr->b) < std::tie(rhs.ptr->mask, rhs.ptr->a, rhs.ptr->b);` (you'll need to `#include ` of course) – Toby Speight Aug 03 '15 at 16:43
  • 1
    @TobySpeight: not seen the bitfield :-(, we can still use `make_tuple` instead of `std::tie` here. – Jarod42 Aug 03 '15 at 17:20
  • @TobySpeight I use unsigned int because the I can switch case 0: 1: 2: 3:... 7: (i think its harder with bitfield) because it's a "mask" but not for bit a bit but for full value. Maybe calling it ID would be better? I'll think a better name next time. – Umuril Lyerood Aug 03 '15 at 18:08
  • Another thing, I don't like ptrcomp struct. There is a way to add the compare inside the weighted pointer one and make it useful for std::set, could you post me how would you do it with tuple? (liked the idea) or should I make another question? – Umuril Lyerood Aug 03 '15 at 18:13
  • I wasn't criticising your `mask` member - simply that you can't make a reference from a bitfield member. @Jarod's suggestion of copying (with `make_tuple`) is a good one. – Toby Speight Aug 04 '15 at 07:26

3 Answers3

10

Your struct here:

struct weighted_pointer{
    mutable int weight;
    unique_ptr<likeatree> ptr;
};

contains a std::unique_ptr. A std::unique_ptr cannot be copied, so your entire weighted_pointer cannot be copied as well.

There are three places in your code where you attempt to copy it, which causes the errors you see:

bool operator()(const weighted_pointer lhs, const weighted_pointer rhs) {

Must be:

bool operator()(weighted_pointer const& lhs, weighted_pointer const& rhs) {
stdSet.insert(tmp);

This could theoretically be fixed by:

stdSet.insert(std::move(tmp));

However, you then cannot use tmp anymore, which you do, not only in the same loop but also in the loop below. So you must find a different solution altogether. Perhaps use emplace. Or restructure your code entirely.

auto it = find_if(stdSet.begin(),stdSet.end(),[&](weighted_pointer temp){ return temp.ptr.get() == treeVector[i]; });

Must be:

auto it = find_if(stdSet.begin(),stdSet.end(),[&](weighted_pointer const& temp){ return temp.ptr.get() == treeVector[i]; });

For VC++ 2013, the std::move fix will not suffice. You will have to add an explicit move constructor to your struct:

struct weighted_pointer{
    mutable int weight;
    unique_ptr<likeatree> ptr;

    weighted_pointer() = default;
    weighted_pointer(weighted_pointer&& src) :
        weight(std::move(src.weight)),
        ptr(std::move(src.ptr))
    {
    }
};

VC++ 2015 fixes this problem. More information: Default Move Constructor in Visual Studio 2013 (Update 3)

Community
  • 1
  • 1
Christian Hackl
  • 27,051
  • 3
  • 32
  • 62
  • Now I realize. I forgot about without reference it copies it. In my mind I was referring it but at the time of writing code I forgot to do it :P I'll change all when I get home but that's probably the main problem. – Umuril Lyerood Aug 03 '15 at 17:59
4

Your weighted_pointer is non-copyable because it contains a non-copyable member (the unique_ptr), so you have to pass it by const reference to your comparator function.

bool operator()(const weighted_pointer& lhs, const weighted_pointer& rhs)

This is because if you pass it by value (as it is currently written), it will try to make a function-local copy.

You also cannot do this because you are trying to copy tmp, which as I just said that struct is non-copyable.

for(unsigned int i = 0; i < stdDeque.size(); i++){
    tmp.ptr.reset(new likeatree{0,&stdDeque[i],nullptr});
    stdSet.insert(tmp);
}

You can use emplace to construct a weighted_pointer in-place instead

for(unsigned int i = 0; i < stdDeque.size(); i++){
    stdSet.emplace(1, std::make_unique<likeatree>(0,&stdDeque[i],nullptr));
}
Cory Kramer
  • 114,268
  • 16
  • 167
  • 218
  • still getting [Error] use of deleted function 'weighted_pointer::weighted_pointer(const weighted_pointer&)' – Umuril Lyerood Aug 03 '15 at 16:24
  • @UmurilLyerood I edited my post, you have another place you are trying to copy this class. Your error message will indicate the offending line, if you see any more messages like this, ensure the class is not being copied anywhere in your code. – Cory Kramer Aug 03 '15 at 16:28
  • [Error] 'make_unique' was not declared in this scope. c++11 doesn't allow it, but np. I changed it to c++14. The tmp variable is used below too. I can't do it in-place. – Umuril Lyerood Aug 03 '15 at 16:38
  • 1
    @UmurilLyerood: You either have to move `tmp` and restructure your code such that the moved-from object is then not used anymore, or use `emplace`. Or not use `std::unique_ptr`. – Christian Hackl Aug 03 '15 at 16:43
0

When I compile the code above, the compiler says:

In file included from /usr/include/c++/4.8/algorithm:62:0,
                 from 31791982.cpp:7:
/usr/include/c++/4.8/bits/stl_algo.h: In instantiation of ‘_InputIterator std::__find_if(_InputIterator, _InputIterator, _Predicate, std::input_iterator_tag) [with _InputIterator = std::_Rb_tree_const_iterator<weighted_pointer>; _Predicate = main()::__lambda0]’:
/usr/include/c++/4.8/bits/stl_algo.h:4465:41:   required from ‘_IIter std::find_if(_IIter, _IIter, _Predicate) [with _IIter = std::_Rb_tree_const_iterator<weighted_pointer>; _Predicate = main()::__lambda0]’
31791982.cpp:55:124:   required from here

So look at line 55 - what's happening:

    auto it = find_if(stdSet.begin(),stdSet.end(),[&](weighted_pointer temp){ return temp.ptr.get() == treeVector[i]; });

We're trying to copy a weighted_pointer from the array into the lambda's temp. But really, we'd be happy with a const ref, so replace with const weighted_pointer& and compile again:

/usr/include/c++/4.8/bits/stl_tree.h: In instantiation of ‘std::pair<std::_Rb_tree_node_base*, std::_Rb_tree_node_base*> std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_M_get_insert_unique_pos(const key_type&) [with _Key = weighted_pointer; _Val = weighted_pointer; _KeyOfValue = std::_Identity<weighted_pointer>; _Compare = ptrcomp; _Alloc = std::allocator<weighted_pointer>; std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::key_type = weighted_pointer]’:
/usr/include/c++/4.8/bits/stl_tree.h:1377:47:   required from ‘std::pair<std::_Rb_tree_iterator<_Val>, bool> std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_M_insert_unique(_Arg&&) [with _Arg = const weighted_pointer&; _Key = weighted_pointer; _Val = weighted_pointer; _KeyOfValue = std::_Identity<weighted_pointer>; _Compare = ptrcomp; _Alloc = std::allocator<weighted_pointer>]’
/usr/include/c++/4.8/bits/stl_set.h:463:29:   required from ‘std::pair<typename std::_Rb_tree<_Key, _Key, std::_Identity<_Key>, _Compare, typename _Alloc::rebind<_Key>::other>::const_iterator, bool> std::set<_Key, _Compare, _Alloc>::insert(const value_type&) [with _Key = weighted_pointer; _Compare = ptrcomp; _Alloc = std::allocator<weighted_pointer>; typename std::_Rb_tree<_Key, _Key, std::_Identity<_Key>, _Compare, typename _Alloc::rebind<_Key>::other>::const_iterator = std::_Rb_tree_const_iterator<weighted_pointer>; std::set<_Key, _Compare, _Alloc>::value_type = weighted_pointer]’
31791982.cpp:49:26:   required from here

Line 49 is:

    stdSet.insert(tmp);

We can't copy tmp into the set. If we weren't going to re-use tmp we could move it instead:

for(unsigned int i = 0; i < stdDeque.size(); i++){
    weighted_pointer tmp;
    tmp.weight = 1;
    tmp.ptr.reset(new likeatree{0,&stdDeque[i],nullptr});
    stdSet.insert(std::move(tmp));
}

That then leaves us with an easy fix for ptrcomp::operator() which needs to accept its arguments by const reference.

Toby Speight
  • 27,591
  • 48
  • 66
  • 103