C++11 problem when inserting into std::unordered_map

    explicit Any(T& value) { Val = new Container<T>(value); }
    ~Any() { if(Val != nullptr) delete Val; }
    Any(const Any&) = default;               // Copy constructor
    Any(Any&&) = default;                    // Move constructor
    Any& operator=(const Any&) & = default;  // Copy assignment operator
    Any& operator=(Any&&) & = default;       // Move assignment operator

You've got a class that manages a pointer resource — the rule of three (which is really the rule of five in C++11) still applies. Using the defaults here is utterly wrong and broken. If you try to copy or move an instance of Any, then you get two instances pointing to a single resource. That means you get two destructors called, which means a double-free when the second one is called.

And you are creating several copies/moves:

    auto result = Impl::Properties->insert(std::make_pair<std::string, Impl::Any>(name.c_str(), 
            Impl::Any(value)));

Putting aside C++11, this results in at least two copies of an Any, most likely three. The Any instance is constructed, then copied into the pair, then copied into another pair that's created as an implicit conversion (because the actual pair type used by the map has a const key), and then finally copied into the container. In C++11 all those copies can be turned into moves, but you're using the default move constructor, which just calls the copy constructor, so it's still a bunch of copies. You need to fix your type so that it is properly copyable and moveable; using the defaults won't do. Any type used with a standard container has to be at minimum copy-assignable, and your type isn't.

If you want to avoid this copying/moving, use the emplace interface, but you still need to fix your type because who knows where a temporary will be created.

Further notes:

  • Use an initializer list:

    explicit Any(T& value) : Val(new Container<T>(value)) { }
    
  • delete on a null pointer is well defined (it does nothing):

    ~Any() { delete Val; }
    
  • This does not mean what you think it does:

    template<typename T> T* const getValue()
    

    That's requesting a return type of "const pointer to modifiable T", but that const qualifier is discarded because conceptually you are making a copy of this pointer when you return it, and that pointer is not const. You probably meant const T*. Once you make that change, Add has to return const T&, which is what it should return anyway.

  • You don't have to write things like if(ptr == nullptr) or if(ptr != nullptr). Just write if(ptr) and if(!ptr).

  • I really don't see why you're making Properties a pointer, with all the extra work necessary to allocate and deallocate it. Just make it a value.

  • You certainly don't want to be using str.c_str() like that. You want to move the string, and that prevents it. Use std::move.

  • You can let the template arguments be deduced when calling make_pair. If you use emplace, then you don't even need to call make_pair at all.

/r/cpp_questions Thread