Storing non copyable object with no default constructor in map (C++11)

Question

I'm trying to implement a class that represents a connection to a file, hence it should be a non-copyable class. Also, since a filename is required to create the object, I'd like to remove the default constructor.

Here is a simplified definition of such a class :

class Elem
{
public:
    Elem() = delete;
    Elem(string name) : file(new ifstream(name,ios::in)) {}

    Elem(const Elem&) = delete;
    Elem& operator=(const Elem& o) = delete;

    Elem& operator=(Elem && o)
    {
        swap(o.file,file);
        o.file = nullptr;
    }
    Elem(Elem &&o) : file(nullptr)
    {
        swap(file,o.file);
    }

    ~Elem()
    {
        if(file!=nullptr){
            file->close();
            delete file;
        }
    }
protected:
    ifstream* file;
};

However, when I try to store such objects in a standard map, the code fails to compile :

int main(){
    map<string,Elem> m;
    m["file1"] = move(Elem("file1"));
    cout<<m.size()<<endl;
    return 0;
}

I get the following errors (gcc 4.9.1) :

In file included from /usr/include/c++/4.9.1/bits/stl_map.h:63:0,
                 from /usr/include/c++/4.9.1/map:61,
                 from test.cpp:1:
/usr/include/c++/4.9.1/tuple: In instantiation of ‘std::pair<_T1, _T2>::pair(std::tuple<_Args1 ...>&, std::tuple<_Args2 ...>&, std::_Index_tuple<_Indexes1 ...>, std::_Index_tuple<_Indexes2 ...>) [with _Args1 = {std::basic_string<char, std::char_traits<char>, std::allocator<char> >&&}; unsigned int ..._Indexes1 = {0u}; _Args2 = {}; unsigned int ..._Indexes2 = {}; _T1 = const std::basic_string<char>; _T2 = Elem]’
/usr/include/c++/4.9.1/tuple:1088:63:   required from ‘std::pair<_T1, _T2>::pair(std::piecewise_construct_t, std::tuple<_Args1 ...>, std::tuple<_Args2 ...>) [with _Args1 = {std::basic_string<char, std::char_traits<char>, std::allocator<char> >&&}; _Args2 = {}; _T1 = const std::basic_string<char>; _T2 = Elem]’
/usr/include/c++/4.9.1/ext/new_allocator.h:120:4:   required from ‘void __gnu_cxx::new_allocator<_Tp>::construct(_Up*, _Args&& ...) [with _Up = std::pair<const std::basic_string<char>, Elem>; _Args = {const std::piecewise_construct_t&, std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >&&>, std::tuple<>}; _Tp = std::_Rb_tree_node<std::pair<const std::basic_string<char>, Elem> >]’
/usr/include/c++/4.9.1/bits/alloc_traits.h:253:4:   required from ‘static std::_Require<typename std::allocator_traits<_Alloc>::__construct_helper<_Tp, _Args>::type> std::allocator_traits<_Alloc>::_S_construct(_Alloc&, _Tp*, _Args&& ...) [with _Tp = std::pair<const std::basic_string<char>, Elem>; _Args = {const std::piecewise_construct_t&, std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >&&>, std::tuple<>}; _Alloc = std::allocator<std::_Rb_tree_node<std::pair<const std::basic_string<char>, Elem> > >; std::_Require<typename std::allocator_traits<_Alloc>::__construct_helper<_Tp, _Args>::type> = void]’
/usr/include/c++/4.9.1/bits/alloc_traits.h:399:57:   required from ‘static decltype (_S_construct(__a, __p, (forward<_Args>)(std::allocator_traits::construct::__args)...)) std::allocator_traits<_Alloc>::construct(_Alloc&, _Tp*, _Args&& ...) [with _Tp = std::pair<const std::basic_string<char>, Elem>; _Args = {const std::piecewise_construct_t&, std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >&&>, std::tuple<>}; _Alloc = std::allocator<std::_Rb_tree_node<std::pair<const std::basic_string<char>, Elem> > >; decltype (_S_construct(__a, __p, (forward<_Args>)(std::allocator_traits::construct::__args)...)) = <type error>]’
/usr/include/c++/4.9.1/bits/stl_tree.h:423:42:   required from ‘std::_Rb_tree_node<_Val>* std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_M_create_node(_Args&& ...) [with _Args = {const std::piecewise_construct_t&, std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >&&>, std::tuple<>}; _Key = std::basic_string<char>; _Val = std::pair<const std::basic_string<char>, Elem>; _KeyOfValue = std::_Select1st<std::pair<const std::basic_string<char>, Elem> >; _Compare = std::less<std::basic_string<char> >; _Alloc = std::allocator<std::pair<const std::basic_string<char>, Elem> >; std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_Link_type = std::_Rb_tree_node<std::pair<const std::basic_string<char>, Elem> >*]’
/usr/include/c++/4.9.1/bits/stl_tree.h:1790:64:   required from ‘std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::iterator std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::_M_emplace_hint_unique(std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::const_iterator, _Args&& ...) [with _Args = {const std::piecewise_construct_t&, std::tuple<std::basic_string<char, std::char_traits<char>, std::allocator<char> >&&>, std::tuple<>}; _Key = std::basic_string<char>; _Val = std::pair<const std::basic_string<char>, Elem>; _KeyOfValue = std::_Select1st<std::pair<const std::basic_string<char>, Elem> >; _Compare = std::less<std::basic_string<char> >; _Alloc = std::allocator<std::pair<const std::basic_string<char>, Elem> >; std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::iterator = std::_Rb_tree_iterator<std::pair<const std::basic_string<char>, Elem> >; std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, _Alloc>::const_iterator = std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char>, Elem> >]’
/usr/include/c++/4.9.1/bits/stl_map.h:519:8:   required from ‘std::map<_Key, _Tp, _Compare, _Alloc>::mapped_type& std::map<_Key, _Tp, _Compare, _Alloc>::operator[](std::map<_Key, _Tp, _Compare, _Alloc>::key_type&&) [with _Key = std::basic_string<char>; _Tp = Elem; _Compare = std::less<std::basic_string<char> >; _Alloc = std::allocator<std::pair<const std::basic_string<char>, Elem> >; std::map<_Key, _Tp, _Compare, _Alloc>::mapped_type = Elem; std::map<_Key, _Tp, _Compare, _Alloc>::key_type = std::basic_string<char>]’
test.cpp:41:14:   required from here
/usr/include/c++/4.9.1/tuple:1099:70: erreur: use of deleted function ‘Elem::Elem()’
         second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)
                                                                  ^
test.cpp:11:5: note: declared here
     Elem() = delete;

Any clues about how this could be achieved ? Are their other design flaws in my structure ?

Thanks in advance

`operator[]` requires default-constructibility: If the key is not present, a node is constructed where the value is value-initialized (calls the default ctor). `operator[]` has to return a reference to a valid object, so it either has to use an existing one or create one. Use `lower_bound`, `find`, and `insert` instead. — dyp, Nov 22 '14 at 23:22
@Deduplicator `insert` should have a move-overload, but yes, `emplace` should also be in that list. (For `vector`, Howard Hinnant showed that `insert` can sometimes be faster than `emplace`; not sure about `map`s, though.) — dyp, Nov 22 '14 at 23:26
@dyp: Any chance for a link to that, or a hint under what circumstances? — Deduplicator, Nov 22 '14 at 23:28
Your move assignment operator is wrong and will leak a stream (`o.file = nullptr;` shouldn't be there). Also from this code `file == nullptr` is a valid state, so why no default constructor? — T.C., Nov 22 '14 at 23:28
@Deduplicator Unfortunately, [Time Warner has forced Hinnant to put his website down](http://stackoverflow.com/users/576911/howard-hinnant) (or give up the domain); some of it is restored here: http://howardhinnant.github.io/ but the paper itself is not directly available there. The paper can be found in source version here: https://github.com/HowardHinnant/papers/blob/master/insert_vs_emplace.html and there are services to directly present it to your browser as HTML. — dyp, Nov 22 '14 at 23:32

Barry · Accepted Answer · 2014-11-22T23:56:39.173

What you're doing is:

map<string,Elem> m;
m["file1"] =              // this default-constructs an Elem 
                          // and returns a reference to it in the correct spot                         
    move(Elem("file1"));  // and THEN move-assigns it

What you need to do:

map<string, Elem> m;
m.emplace("file1", Elem("file1"));

or

m.insert(std::make_pair("file1", Elem("file1"));

or

m.emplace(std::piecewise_construct,
      std::forward_as_tuple("file1"),
      std::forward_as_tuple("file1"));

As for your design, the comments point out the issue that you're potentially leaking a stream. But this is actually a really easy fix. What you are expressing is unique ownership over a file, where that ownership is transferable. There's a type for that: unique_ptr!

class Elem {
    std::unique_ptr<ifstream> file;

public:
    Elem() = delete;
    Elem(string name) : file(new ifstream(name,ios::in)) {}

    Elem(Elem&& ) = default;
    Elem& operator=(Elem&& ) = default;
    ~Elem() = default;

    Elem(const Elem&) = delete;
    Elem& operator=(const Elem&) = delete;
};

Using unique_ptr makes those default/delete lines not even necessary (except the default constructor one - since that's a requirement you're enforcing), which is even better (see Rule of Zero). I'm just illustrating them for clarity.

Still a move-ctor or copy-ctor. Why not give the ctor-argument itself? — Deduplicator, Nov 22 '14 at 23:29
@Deduplicator do you need `move(Elem("file1"))` to make insert work? I thought there was a way to still use `insert` for noncopyable types. — Barry, Nov 22 '14 at 23:32
@Barry Thanks for the explanations, could you please elaborate on what you mean by "leaking a stream" ? (although I understand that using unique_ptr is better) — alcinos, Nov 23 '14 at 18:32
@alcinos In your move-operator, if you do `elem = Elem("newfile")`, then `elem` will have a pointer to the new file, but the old file never gets closed or deleted... you just set it to `nullptr`. The file is still open, but nobody owns a pointer to it. Hence, leaked. — Barry, Nov 23 '14 at 19:17

Storing non copyable object with no default constructor in map (C++11)

1 Answers1