search

pmem / libpmemobj-cpp

C++ bindings & containers for libpmemobj
https://pmem.io
Other
107 stars 76 forks source link

Concurrent map get_allocator compilation error #827

Open KFilipek opened 4 years ago

KFilipek commented 4 years ago

Hi, I've got conversion issue in this PR: #825 @vinser52, can you help with this problem?

Environment Information

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

igchor commented 4 years ago

@vinser52 could you look at this? I think there is some type mismatch (_node_allocator vs allocator_type).

vinser52 commented 4 years ago

I will have a look today.

vinser52 commented 4 years ago

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?

vinser52 commented 4 years ago

@igchor Do you remember why we decided to return by reference. I remember we had some discussion about that, but I forgot details.

igchor commented 4 years ago

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:

  1. 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.
  2. Change the return type of get_allocator() or change get_allocator() to something like get_node_allocator() function
vinser52 commented 4 years ago

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.

igchor commented 4 years ago

Ok, let's do that, and maybe let's also put a comment with link to this issue.

vinser52 commented 4 years ago

@KFilipek will you do this?

KFilipek commented 4 years ago

Sure, I've updated the PR #825 with proper changes.

KFilipek commented 4 years ago

@igchor Related PR (#825 ) was merged to the master branch. Can we close this issue?

igchor commented 4 years ago

I think we should keep it open (but probably not as a bug), since the current solution is mostly a workaround.