static_assert on the constructibility of the containers' types

[k3DW]

Closes #191

It turns out type_policy::construct() is the common place where we hit errors when the types are not move-constructible. This change encompasses all problematic places like .emplace() or .insert(), but this does not affect the non-problematic places like the default constructor.

[cmazakas]

Hmm, maybe I'm missing something but doesn't it make more sense to just do like:

template<class Key, ....>
class unordered_flat_map 
{
    static_assert(std::is_move_constructible<value_type>::value, "");
};

?

Note the original issues says:

Unfortunately, if you use a type that is not move-constructible, the compiler generates an enormous wall of cryptic errors instead... because the compilation failure occurs inside of a deep chain of template instantiations.

I think putting the static_assert() far away from the deep internal implementation details might lead to nicer error messages but I haven't tested locally.

Braden, do you know what some of the error output looks like with your current PR vs what it'd look like with my suggestion?

[k3DW]

Braden, do you know what some of the error output looks like with your current PR vs what it'd look like with my suggestion?

The error output would look the same, but errors would occur in more places with your suggestion. Things like the default constructor, calling .size(), etc, all of these functions don't currently cause a compilation error, and that behaviour is maintained with my suggestion in this PR. The functions that do cause compilation errors with cryptic error messages instead get this nice static_assert.

I was under the assumption that I should maintain the current behaviour, but I'm open to putting the static_assert in the class body instead.

[cmazakas]

Thinking about it some more, I think we need:

          template <class A, class... Args>
          static void construct(A& al, init_type* p, Args&&... args)
          {
            // note we're checking `init_type` here
            static_assert(std::is_move_constructible<init_type>::value,
              "key_type and mapped_type must be move constructible");
            boost::allocator_construct(al, p, std::forward<Args>(args)...);
          }

          template <class A, class... Args>
          static void construct(A& al, value_type* p, Args&&... args)
          {
            static_assert(std::is_move_constructible<value_type>::value,
              "key_type and mapped_type must be move constructible");
            boost::allocator_construct(al, p, std::forward<Args>(args)...);
          }

          template <class A, class... Args>
          static void construct(A& al, key_type* p, Args&&... args)
          {
            static_assert(std::is_move_constructible<key_type>::value,
              "key_type must be move constructible");
            boost::allocator_construct(al, p, std::forward<Args>(args)...);
          }

Note that the docs also cite value_type must be move-constructible: https://www.boost.org/doc/libs/develop/libs/unordered/doc/html/unordered.html#compliance_open_addressing_containers

I think it's only valid to check the key_type by itself for the last overload of construct().

It also seems like the STL will delay the static_assert() in a similar fashion. I suppose the motivation is to simply not break builds, regardless of how silly the code winds up being.

[k3DW]

Thinking about it some more, I think we need: ...

I tried this at first, but it broke some of the tests. Sometimes value_type is not move constructible even when key_type is, so checking for std::is_move_constructible<value_type> will be incorrect (Compiler Explorer link). However, it needs to be checked in the value_type overload of construct() otherwise it doesn't catch the errors in the right places.

If anything, I'd suggest we check for both key_type and mapped_type being move constructible in all 3 places. Those are the actual constraints, and anything else just falls out of that.

Note that the docs also cite value_type must be move-constructible:

Ah I see. Well I've found that this isn't true. I think the key_type and mapped_type must be move constructible, but the value_type sometimes isn't.

[joaquintides]

Several observations about the current status of this PR:

• Type policies are the right place to locate these assertions, as key/value construction always go through them. Note, however, that node policies node_map_types.hpp and node_set_types.hpp haven't been instrumented (and they should).
• Strictly speaking, the containers (as all STL containters) do not impose, say, move constructibility but move insertability, involving a construction call to the allocator. In general, we can't static assert with any allocator cause their internals are opaque to us, so these assertions need be restricted to std::allocator or, if we want to be exquisite, to allocators without a defined constuct member function.

I'd organize all of this as follows:

• Have all four type policies delegate assertion to two static assertion facilities (one for maps, one for sets).
• These facilities won't do anything if the allocator is not std::allocator (or, as mentioned above, if the allocator has a construct member function).
• They are passed the types Allocator, ConstructedType (any of init_type, value_type or key_type), and Args.... We can then provide as many specializations/overloads as required, for instance:
  • std::allocator, key_type, key_type&& --> assert key_type move constructibility.

Happy to discuss further details/doubts about this approach.

[k3DW]

I think we need to discuss this more outside of these comments, I have some follow-up questions.

[k3DW]

Sorry for all the pushes, I forgot my branch was linked to this PR.

[joaquintides]

I decided to provide a general review rather than in-code comments, I think it's going to be clearer this way:

• In node_(set|map)_types.hpp you're not ever invoking the checker when constructing element_types, only when the user-related init_type, value_type etc. are constructed. This is correct.
• Now, there is no need to have different checkers for flat and node scenarios, as node_(set|map)_types.hpp already make element_type invisible to the checker. That is, you can have one map checker and one set checker alone.
• There's a low of duplication of typedefs as in:

using key_type = Key;
using mapped_type = T;
using raw_key_type = typename std::remove_const<Key>::type;
using raw_mapped_type = typename std::remove_const<T>::type;

This is a code smell. Instead, you can have the checker be parameterized by TypePolicy and extract the typedefs from there, without redefinition.

• static_assert(std::is_constructible<X, Args...>::value, ...) should be static_assert(std::is_constructible<X, Args&&...>::value, ...) (several places).
• The map checker is too coarse. For instance, this is not detected by the checker:

boost::unordered_node_map<nonmoveable,int> x;
x.emplace(nonmoveable{},0);

This is because emplacement is done through std::piecewise_construct, and the coresponding std::pair constructor is not SFINAE-enabled (at least in MSVC), so the checker happily passes it. To make the map checker really powerful, we need to provide check overloads for

(value_type*, Arg1&&, Arg2&&)
(value_type*, const std::pair<Arg1, Arg2>&)
(value_type*, std::pair<Arg1, Arg2>&&)
(value_type*, std::piecewise_construct&&, std::tuple<Arg1>&&, std::tuple<Arg2>&&,)
(value_type*, std::piecewise_construct&&, std::tuple<Arg1>&&, std::tuple<Args2...>&&,)
(value_type*, std::piecewise_construct&&, std::tuple<Args1...>&&, std::tuple<Args2>&&,)

(and same for init_type*) and then internally derive to individual checkers for the key and mapped part. Incidentally, this strategy renders the current (value_type*, moved_type&&) and (init_type*, moved_type&&) overloads superfluous and will provide more informative error messages than "value_type must be move constructible".

[k3DW]

Alright, I implemented something similar to what you suggested. I wanted to ensure your non-functioning example not only became functional, but also that I specifically give error messages about invalid move construction separately from the other error messages. I'm sure that's the most common error of this type to be hit. I think the users will appreciate seeing key_type is not move constructible, instead of seeing key_type is not constructible from Args when Args is just key_type&& anyway.

[k3DW]

I got a verbal approval from @cmazakas outside of these comments. I'll go ahead and merge.