Better-enum in maps

[biziosan]

Hi there,

Thank you for your library! I have the following issue, and I am wondering if there is something I am missing.
When I compile (GCC 9.3 - Linux) this simple piece of code:

BETTER_ENUM(encodings_e, int,
    RED,
    GREEN,
    BLUE
)

std::unordered_map<std::string, encodings_e> map;

map["rr"] = encodings_e::RED;
encodings_e foo = map["rr"];
std::cout << foo._to_string() << std::endl;

I get the following error:

In file included from /usr/include/c++/9/bits/unique_ptr.h:37,
                 from /usr/include/c++/9/memory:80,
                 from /usr/include/gtest/gtest.h:57,
                 from /home/veesai/aicore/src/foundation/tests/enums/enums.cpp:5:
/usr/include/c++/9/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::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&}; long unsigned int ..._Indexes1 = {0}; _Args2 = {}; long unsigned int ..._Indexes2 = {}; _T1 = const std::__cxx11::basic_string<char>; _T2 = encodings_e]’:
/usr/include/c++/9/tuple:1663:63:   required from ‘std::pair<_T1, _T2>::pair(std::piecewise_construct_t, std::tuple<_Args1 ...>, std::tuple<_Args2 ...>) [with _Args1 = {std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&}; _Args2 = {}; _T1 = const std::__cxx11::basic_string<char>; _T2 = encodings_e]’
/usr/include/c++/9/ext/new_allocator.h:147:4:   required from ‘void __gnu_cxx::new_allocator<_Tp>::construct(_Up*, _Args&& ...) [with _Up = std::pair<const std::__cxx11::basic_string<char>, encodings_e>; _Args = {const std::piecewise_construct_t&, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&>, std::tuple<>}; _Tp = std::__detail::_Hash_node<std::pair<const std::__cxx11::basic_string<char>, encodings_e>, true>]’
/usr/include/c++/9/bits/alloc_traits.h:484:4:   required from ‘static void std::allocator_traits<std::allocator<_Tp1> >::construct(std::allocator_traits<std::allocator<_Tp1> >::allocator_type&, _Up*, _Args&& ...) [with _Up = std::pair<const std::__cxx11::basic_string<char>, encodings_e>; _Args = {const std::piecewise_construct_t&, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&>, std::tuple<>}; _Tp = std::__detail::_Hash_node<std::pair<const std::__cxx11::basic_string<char>, encodings_e>, true>; std::allocator_traits<std::allocator<_Tp1> >::allocator_type = std::allocator<std::__detail::_Hash_node<std::pair<const std::__cxx11::basic_string<char>, encodings_e>, true> >]’
/usr/include/c++/9/bits/hashtable_policy.h:2086:36:   required from ‘std::__detail::_Hashtable_alloc<_NodeAlloc>::__node_type* std::__detail::_Hashtable_alloc<_NodeAlloc>::_M_allocate_node(_Args&& ...) [with _Args = {const std::piecewise_construct_t&, std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&&>, std::tuple<>}; _NodeAlloc = std::allocator<std::__detail::_Hash_node<std::pair<const std::__cxx11::basic_string<char>, encodings_e>, true> >; std::__detail::_Hashtable_alloc<_NodeAlloc>::__node_type = std::__detail::_Hash_node<std::pair<const std::__cxx11::basic_string<char>, encodings_e>, true>]’
/usr/include/c++/9/bits/hashtable_policy.h:726:8:   required from ‘std::__detail::_Map_base<_Key, _Pair, _Alloc, std::__detail::_Select1st, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits, true>::mapped_type& std::__detail::_Map_base<_Key, _Pair, _Alloc, std::__detail::_Select1st, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits, true>::operator[](std::__detail::_Map_base<_Key, _Pair, _Alloc, std::__detail::_Select1st, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits, true>::key_type&&) [with _Key = std::__cxx11::basic_string<char>; _Pair = std::pair<const std::__cxx11::basic_string<char>, encodings_e>; _Alloc = std::allocator<std::pair<const std::__cxx11::basic_string<char>, encodings_e> >; _Equal = std::equal_to<std::__cxx11::basic_string<char> >; _H1 = std::hash<std::__cxx11::basic_string<char> >; _H2 = std::__detail::_Mod_range_hashing; _Hash = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<true, false, true>; std::__detail::_Map_base<_Key, _Pair, _Alloc, std::__detail::_Select1st, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits, true>::mapped_type = encodings_e; std::__detail::_Map_base<_Key, _Pair, _Alloc, std::__detail::_Select1st, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits, true>::key_type = std::__cxx11::basic_string<char>]’
/usr/include/c++/9/bits/unordered_map.h:989:20:   required from ‘std::unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::mapped_type& std::unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::operator[](std::unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::key_type&&) [with _Key = std::__cxx11::basic_string<char>; _Tp = encodings_e; _Hash = std::hash<std::__cxx11::basic_string<char> >; _Pred = std::equal_to<std::__cxx11::basic_string<char> >; _Alloc = std::allocator<std::pair<const std::__cxx11::basic_string<char>, encodings_e> >; std::unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::mapped_type = encodings_e; std::unordered_map<_Key, _Tp, _Hash, _Pred, _Alloc>::key_type = std::__cxx11::basic_string<char>]’
/home/veesai/aicore/src/foundation/tests/enums/enums.cpp:22:13:   required from here
/usr/include/c++/9/tuple:1674:70: error: ‘encodings_e::encodings_e()’ is private within this context
 1674 |         second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)
      |                                                                      ^
In file included from /home/veesai/aicore/src/foundation/tests/../include/foundation/types/enum_class.hpp:7,
                 from /home/veesai/aicore/src/foundation/tests/../include/foundation/types.hpp:11,
                 from /home/veesai/aicore/src/foundation/tests/enums/enums.cpp:7:
/home/veesai/aicore/src/foundation/tests/enums/enums.cpp:11:13: note: declared private here
   11 | BETTER_ENUM(encodings_e, int,
      |             ^~~~~~~~~~~
/home/veesai/aicore/src/foundation/tests/../include/vendors/better_enums/better_enum.hpp:156:28: note: in definition of macro ‘BETTER_ENUMS_ID’
  156 | #define BETTER_ENUMS_ID(x) x
      |                            ^
/home/veesai/aicore/src/foundation/tests/../include/vendors/better_enums/better_enum.hpp:716:5: note: in expansion of macro ‘BETTER_ENUMS_DEFAULT_CONSTRUCTOR’
  716 |     BETTER_ENUMS_DEFAULT_CONSTRUCTOR(Enum)                                     \
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/veesai/aicore/src/foundation/tests/../include/vendors/better_enums/better_enum.hpp:1193:21: note: in expansion of macro ‘BETTER_ENUMS_TYPE’
 1193 |     BETTER_ENUMS_ID(BETTER_ENUMS_TYPE(                                         \
      |                     ^~~~~~~~~~~~~~~~~
/home/veesai/aicore/src/foundation/tests/enums/enums.cpp:11:1: note: in expansion of macro ‘BETTER_ENUM’
   11 | BETTER_ENUM(encodings_e, int,
      | ^~~~~~~~~~~
make[3]: *** [tests/CMakeFiles/test_enums.dir/build.make:83: tests/CMakeFiles/test_enums.dir/enums/enums.cpp.o] Error 1
make[2]: *** [CMakeFiles/Makefile2:199: tests/CMakeFiles/test_enums.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:206: tests/CMakeFiles/test_enums.dir/rule] Error 2
make: *** [Makefile:234: test_enums] Error 2

Line 22 where the error happens is on the insertion of the enum in the map.

[aantron]

It's been a long time, but it looks like there is an intermediate call to a default constructor somewhere during the insert.

I'm not immediately sure what the best solution to this is, but you should be able to work around it in at least one way by declaring the constructor public as desribed here: https://aantron.github.io/better-enums/OptInFeatures.html#DefaultConstructors.

[jaskij]

Use emplace

BETTER_ENUM(encodings_e, int,
    RED,
    GREEN,
    BLUE
)

std::unordered_map<std::string, encodings_e> map;

map.emplace(std::make_pair("rr", encodings_e::RED);
encodings_e foo = map["rr"];
std::cout << foo._to_string() << std::endl;

Edit:

@aantron come to think of it, I believe emplace is the only way to avoid using a default-constructed temporary when inserting into standard containers, maybe you could add that info to documentation?

[aantron]

@jaskij Thanks!

Let's see if there is any word from @biziosan on whether emplace fixes the issue, and then I'll add it to the docs.

[jaskij]

@aantron since I'm dealing with Better Enums now anyway I've tested this, also using gcc 9.3.

TL;DR: operator[] for std::map and std::unordered_map requires the value type to be default constructible.

The result is that std::unordered_map::operator[] (and likely std::map::operator[] too) won't work. The reason is that operator[] looks up the key and inserts a default-constructed value if the key is not present in the map. Even if we don't use it that way the compiler can't instantiate the operator if the value type is not default constrible.

Also, consider the following notes from operator[] description on cppreference.com:

In the published C++11 and C++14 standards, this function was specified to require mapped_type to be DefaultInsertable and key_type to be CopyInsertable or MoveInsertable into *this. This specification was defective and was fixed by LWG issue 2469, and the description above incorporates the resolution of that issue.

However, one implementation (libc++) is known to construct the key_type and mapped_type objects via two separate allocator construct() calls, as arguably required by the standards as published, rather than emplacing a value_type object.

operator[] is non-const because it inserts the key if it doesn't exist. If this behavior is undesirable or if the container is const, at() may be used.

All in all, insert via emplace() and access via at(), those are sure to work.

Sample code:

#include <iostream>
#include <map>

#include <better-enums/enum.h>

BETTER_ENUM(mbool, char, mfalse = 0, mtrue = 1)

int main() {
    std::map<std::string, mbool> map;
    map.emplace(std::make_pair("true", mbool::mfalse));
    //const auto t = map["true"]; // compiler error
    const auto t = map.at("true");
    //std::cout << map["true"] << std::endl; // compiler error
    std::cout << map.at("true") << std::endl;
    std::cout << t << std::endl;

    return 0;
}

[biziosan]

I haven't tried emplace() yet, but with the default constructor enabled my simple code works. I'll test emplace() right away as well.

[jaskij]

@biziosan I don't know if you read my last message, but regardless of emplace(), without a public default constructor you can't use operator[] with maps, so that might be a pain point.

[biziosan]

@jaskij Hi, yes, thank you! It matches with I have working. I used @aantron suggestion about public default constructors and enabled them (https://aantron.github.io/better-enums/OptInFeatures.html#DefaultConstructors). That worked right away!

I think I am satisfied with the answers. I can close this issue if you want.