1

So i'm trying to build a template class for binary trees. I have a special way that i want to use to initialize the tree (which you can see in main()).

The constructor works with shared_ptr just fine, but i want to move it to more lightweight unique_ptr, but i somehow get error-messages all over the place. Can someone explain how to get rid of the shared_ptrs? Here is the working version with shared_ptr:

#include <optional>
#include <memory>
#include <string>

template <class C>
class node{
private:
    C label;
    std::shared_ptr<node<C>> left;
    std::shared_ptr<node<C>> right;
public:
    node(const C& name,
            const std::optional<node<C>>& leftChild = {},
            const std::optional<node<C>>& rightChild = {}) : 
        label(name), 
        left(leftChild ? std::make_shared<node<C>>(*leftChild) : nullptr), 
        right(rightChild ? std::make_shared<node<C>>(*rightChild) : nullptr) {}
};

int main(){
    using sNode = node<std::string>;
    auto root = std::make_unique<sNode>("root", sNode("left", sNode("left.left")), sNode("right"));
}

I tried to just use unique instead of shared like that:

template <class C>
class node{
private:
    C label;
    std::unique_ptr<node<C>> left;
    std::unique_ptr<node<C>> right;
public:
    node(const C& name,
            const std::optional<node<C>>& leftChild = {},
            const std::optional<node<C>>& rightChild = {}) : 
        label(name), 
        left(leftChild ? std::make_unique<node<C>>(*leftChild) : nullptr), 
        right(rightChild ? std::make_unique<node<C>>(*rightChild) : nullptr) {}
};

But following compile-errors come up:

In file included from /usr/include/c++/7/memory:80:0,
                 from binaryTree.hpp:2,
                 from main.cpp:1:
/usr/include/c++/7/bits/unique_ptr.h: In instantiation of ‘typename std::_MakeUniq<_Tp>::__single_object std::make_unique(_Args&& ...) [with _Tp = node<std::__cxx11::basic_string<char> >; _Args = {const node<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >&}; typename std::_MakeUniq<_Tp>::__single_object = std::unique_ptr<node<std::__cxx11::basic_string<char> >, std::default_delete<node<std::__cxx11::basic_string<char> > > >]’:
binaryTree.hpp:15:51:   required from ‘node<C>::node(const C&, const std::optional<node<C> >&, const std::optional<node<C> >&) [with C = std::__cxx11::basic_string<char>]’
main.cpp:7:80:   required from here
/usr/include/c++/7/bits/unique_ptr.h:825:30: error: use of deleted function ‘node<std::__cxx11::basic_string<char> >::node(const node<std::__cxx11::basic_string<char> >&)’
     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from main.cpp:1:0:
binaryTree.hpp:5:7: note: ‘node<std::__cxx11::basic_string<char> >::node(const node<std::__cxx11::basic_string<char> >&)’ is implicitly deleted because the default definition would be ill-formed:
 class node{
       ^~~~
binaryTree.hpp:5:7: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = node<std::__cxx11::basic_string<char> >; _Dp = std::default_delete<node<std::__cxx11::basic_string<char> > >]’
In file included from /usr/include/c++/7/memory:80:0,
                 from binaryTree.hpp:2,
                 from main.cpp:1:
/usr/include/c++/7/bits/unique_ptr.h:388:7: note: declared here
       unique_ptr(const unique_ptr&) = delete;
       ^~~~~~~~~~
In file included from main.cpp:1:0:
binaryTree.hpp:5:7: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = node<std::__cxx11::basic_string<char> >; _Dp = std::default_delete<node<std::__cxx11::basic_string<char> > >]’
 class node{
       ^~~~
In file included from /usr/include/c++/7/memory:80:0,
                 from binaryTree.hpp:2,
                 from main.cpp:1:
/usr/include/c++/7/bits/unique_ptr.h:388:7: note: declared here
       unique_ptr(const unique_ptr&) = delete;
       ^~~~~~~~~~

I guess it has to do with the deleted assignement-operator for unique_ptr, and i have to use std::move() sonmewhere, but i cant figure out where. Thank you for your help!

BlueQuote
  • 98
  • 10
  • 1
    But `shared_ptr` is as "light" as possible; why do you feel you need smthg lighter? – curiousguy Sep 21 '19 at 14:02
  • 1
    Because when i'm thinking about it, it doesn't seem like i need "shared" ownership. In my mind every node of a tree has exactly ONE parent, so ```unique_ptr``` seems like the better choice in terms of logical reasons -**Edit**: i feel like the ref count of ```shared_ptr``` is an unnecessary burden. – BlueQuote Sep 21 '19 at 14:12
  • 1
    Indeed. Compared to unique "smart ptr", the shared ones provide at least one additional degree of freedom. It's a good practice to prefer tools with the least degrees of freedom covering your needs: a unique ptr provides a guarantee that there won't be another owner sharing the resource. – curiousguy Sep 21 '19 at 15:12
  • Basically you need to write some code to copy `unique_ptr` but then you are back to square one; that put together will be very likely inferior to `std::share_ptr` – Chef Gladiator Sep 21 '19 at 21:53

1 Answers1

0

but i somehow get error-messages all over the place. Can someone explain how to get rid of the shared_ptrs?

unique_ptr is non-copyable, only movable. This means that as soon as you put in your class a unique_ptr member, node cannot be copied anymore: the implicit copy assignment and constructor that the compiler was generating for you won't be there anymore.

So you have to decide how you want your class to behave.

  • If you want your nodes to be copyable, you will have to implement the copy ops yourself (you will have to deep clone the tree). This will give you value semantics.

  • Otherwise, if you don't want node to be copyable, you will still need to perform a deep clone of the node objects in the constructor. The reason is that since you are passing const references, you cannot take ownership of their memory (and you wanted them const since you likely wanted to be able to pass temporaries).

  • Or you can change your interface to avoid taking references. Instead, take sink parameters (i.e. by value) and move them inside your constructor. (@rafix07 wrote this in an answer but it is now deleted). You can even do it for the name parameter:

    node(C name,
            std::optional<node<C>> leftChild = {},
            std::optional<node<C>> rightChild = {}) : 
        label(std::move(name)), 
        left(leftChild ? std::make_unique<node<C>>( std::move(*leftChild) ) : nullptr), 
        right(rightChild ? std::make_unique<node<C>>( std::move(*rightChild) ) : nullptr) {}
    
Acorn
  • 22,093
  • 4
  • 30
  • 62
  • 1
    `std::optional>` is how I might do it in some scenarios, but word of caution if I may. `std::optional` requires types that are "Trivially Destructible", thus imposing that requirement on both `C` and `node`. But I see nothing wrong with `shared_ptr` it is light and it is fast. – Chef Gladiator Sep 21 '19 at 14:43
  • 1
    @ChefGladiator Note that `shared_ptr` is not light/fast compared to `unique_ptr`. There are three overheads: the size (2x), the ref-counting and the atomics. – Acorn Sep 21 '19 at 16:52
  • @ChefGladiator If you think you can write a "*quick app*" to measure it, you definitely don't know about it. :) The "*few microseconds*" you may measure with a microbenchmark is not the full story. Please make a non-trivial app where cache/memory pressure matters and you will see the difference. – Acorn Sep 21 '19 at 19:13
  • @ChefGladiator C has nothing to do with this and is no different; I am not sure what you are talking about. As for "*inventing requirements*", well... if time/space requirements are not your requirements, why are you using something like C++ to begin with? There far more productive languages when you can relax the constraints. – Acorn Sep 22 '19 at 08:27
  • OP had a simple question, I answered change has no benefit, then you answered it has... please take the way you have chosen. – Chef Gladiator Sep 22 '19 at 11:02
  • 1
    @ChefGladiator `unique_ptr` has some objective advantages that I pointed out and you claimed they aren't worth/measurable. I am not sure what is bad about discussing it. In any case, if you are the one that downvoted, please explain why you did so. The answer is fairly thorough. – Acorn Sep 22 '19 at 11:10
  • @ChefGladiator By the way, `std::optional` does *not* require TriviallyDestructible, just Destructible. – Acorn Sep 22 '19 at 11:54