libpmemobj-cpp
libpmemobj-cpp copied to clipboard
Concurrent map get_allocator compilation error
Hi, I've got conversion issue in this PR: #825 @vinser52, can you help with this problem?
Environment Information
- libpmemobj-cpp version(s): latest
- PMDK (libpmemobj) package version(s): 1.8
- OS(es) version(s): Fedora 31
- kernel version(s): 5.4.0-42-generic
- compiler, libraries, packaging and other related tools version(s): clang-9
Please provide a reproduction of the bug:
Just compile code #825
How often bug is revealed:
always
Actual behavior:
In file included from /opt/workspace/tests/external/libcxx/map/map.access/max_size.pass.cpp:17:
In file included from /opt/workspace/tests/common/map_wrapper.hpp:15:
In file included from /opt/workspace/include/libpmemobj++/experimental/concurrent_map.hpp:8:
/opt/workspace/include/libpmemobj++/container/detail/concurrent_skip_list_impl.hpp:1825:10: error: no viable conversion from returned value of type 'const allocator<unsigned char, standard_alloc_policy<unsigned char>, object_traits<unsigned char>>' to function return type 'const allocator<pmem::detail::pair<const char, int>, standard_alloc_policy<pmem::detail::pair<const char, int>>, object_traits<pmem::detail::pair<const char, int>>>'
return _node_allocator;
^~~~~~~~~~~~~~~
/opt/workspace/tests/external/libcxx/map/map.access/max_size.pass.cpp:75:9: note: in instantiation of member function 'pmem::detail::concurrent_skip_list<pmem::detail::map_traits<char, int, std::less<char>, pmem::detail::default_random_generator, pmem::obj::allocator<pmem::detail::pair<const char, int>, pmem::obj::standard_alloc_policy<pmem::detail::pair<const char, int> >, pmem::obj::object_traits<pmem::detail::pair<const char, int> > >, false, 64> >::get_allocator' requested here
.get_allocator()));
^
/opt/workspace/include/libpmemobj++/allocator.hpp:485:2: note: candidate constructor not viable: no known conversion from 'const pmem::detail::concurrent_skip_list<pmem::detail::map_traits<char, int, std::less<char>, pmem::detail::default_random_generator, pmem::obj::allocator<pmem::detail::pair<const char, int>, pmem::obj::standard_alloc_policy<pmem::detail::pair<const char, int> >, pmem::obj::object_traits<pmem::detail::pair<const char, int> > >, false, 64> >::node_allocator_type' (aka 'const allocator<unsigned char, standard_alloc_policy<unsigned char>, object_traits<unsigned char> >') to 'const pmem::obj::allocator<pmem::detail::pair<const char, int>, pmem::obj::standard_alloc_policy<pmem::detail::pair<const char, int> >, pmem::obj::object_traits<pmem::detail::pair<const char, int> > > &' for 1st argument
allocator(allocator const &rhs) : Policy(rhs), Traits(rhs)
^
1 error generated.
make[2]: *** [tests/external/CMakeFiles/map_libcxx_max_size.dir/build.make:83: tests/external/CMakeFiles/map_libcxx_max_size.dir/libcxx/map/map.access/max_size.pass.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:31328: tests/external/CMakeFiles/map_libcxx_max_size.dir/all] Error 2
Additional information about Priority and Help Requested:
Are you willing to submit a pull request with a proposed change? Yes
Requested priority: High
@vinser52 could you look at this? I think there is some type mismatch (_node_allocator vs allocator_type).
I will have a look today.
I think the issue is that we return allocator_type
object by reference (not by value, like std::map) while internally we are using node_allocator_type
object.
The easiest solution is to store allocator_type
object in addition to node_allocator_type
object inside concurrent_skip_list. But it means that we will use node_allocator_type
object for allocation inside the skip list but get_allocator()
method will return another object.
From the first point of view I do not see any issues with that. What do you think?
@igchor Do you remember why we decided to return by reference. I remember we had some discussion about that, but I forgot details.
I think we changed it along with key_comp to allow altering the allcoator/comparator by the user. It was used in pmemkv to call runtime_initialize() on comparator.
If we want to return reference, we should definitely return reference to the actual allocator which is used for allocations. So we probably have 2 options:
- Change it back to return allocator by value. This won't break anything but I'm not sure if we won;t need reference sometime in future.
- Change the return type of get_allocator() or change get_allocator() to something like get_node_allocator() function
There is third option: Remove get_allocator() method for now. Until we have no use case where it is required I think we can remove it and think how to implement it in a right way.
Ok, let's do that, and maybe let's also put a comment with link to this issue.
@KFilipek will you do this?
Sure, I've updated the PR #825 with proper changes.
@igchor Related PR (#825 ) was merged to the master branch. Can we close this issue?
I think we should keep it open (but probably not as a bug), since the current solution is mostly a workaround.