Fix issue with constructing an FOA container from a class with a template conversion operator

[k3DW]

Closes #331

[joaquintides]

Not entirely sure, but wouldn't a workaround based on boost::unordered::detail::type_identity achieve the same result with less hassle?

[k3DW]

Not entirely sure, but wouldn't a workaround based on boost::unordered::detail::type_identity achieve the same result with less hassle?

Do you mean something like this?

template <bool avoid_explicit_instantiation = true>
unordered_node_map(typename detail::type_identity<
  concurrent_node_map<Key, T, Hash, KeyEqual, Allocator> >::type&& other)
    : table_(std::move(other.table_))
{
}

This doesn't work because the constructor argument isn't dependent on any template argument. But the following also doesn't work.

template <class TT, bool B> struct type_identity_with_bool
{
  using type = TT;
};
template <bool avoid_explicit_instantiation = true>
unordered_node_map(typename type_identity_with_bool<
  concurrent_node_map<Key, T, Hash, KeyEqual, Allocator>,
  avoid_explicit_instantiation>::type&& other)
    : table_(std::move(other.table_))
{
}

I'm not exactly sure why this one doesn't work, but I think it's because of the defaulted bool template argument. I'd appreciate some more suggestions though, if this isn't what you meant. I'm all out of ideas.

[joaquintides]

Do you mean something like this?

template
unordered_node_map(typename detail::type_identity<
concurrent_node_map<Key, T, Hash, KeyEqual, Allocator> >::type&& other)
: table_(std::move(other.table_))
{
}

This doesn't work because the constructor argument isn't dependent on any template argument.

Umm, seems to work for me:

https://godbolt.org/z/hjP83o3eE

Correction: it doesn't work for me either.

[joaquintides]

Maybe this?

      template <
        typename T2,
        typename std::enable_if<std::is_same<T, T2>::value, int>::type = 0
      >
      unordered_node_map(
        concurrent_node_map<Key, T2, Hash, KeyEqual, Allocator>&& other)
          : table_(std::move(other.table_))
      {
      }

[k3DW]

Maybe this?

That will work for the main failing case, but it will change the overload resolution so that you can no longer construct an unordered_node_map from anything that converts to concurrent_node_map. I have no idea whether this would or wouldn't break someone's build, but I think it's better to try and find a solution that keeps the same behaviour as before.

That said, the solution that I currently have up in this PR also doesn't work on Windows, so I'm not done exploring how to fix this.

[k3DW]

At this point, we have multiple options for how to write the 1st constructor, where we want to match exactly an rvalue reference to the CFOA counterpart container.

For the 2nd constructor, this is where the issues are happening. It seems that std::is_convertible<...>::value is causing the other container to be instantiated on certain platforms (Clang 7, GCC >= 10, MSVC >= 14.2). This is the condition that's causing the instantiation.

std::is_convertible<U&&, concurrent_node_map<Key, T, Hash,
                           KeyEqual, Allocator> >::value,

On MSVC 14.3, I got this condition working instead. But it doesn't work on Clang 21, the version I'm testing locally.

std::is_void_v<std::void_t<decltype(std::declval<U&&>()
    .operator concurrent_node_map<Key, T, Hash, KeyEqual,
      Allocator>())> >

The weird thing about this one, it relies on std::is_void_v and cannot use std::is_void<...>::value, otherwise the static_assert will fail in the test. We might be able to write some macro code to recreate the behaviour of std::is_void_v using the platform-specific intrinsic functions, so that might be fine. I'm not sure if this idea is worth pursuing though.

Overall I think it's fine to check for the existence of operator concurrent_node_map(), instead of just general "convertibility". Since we know we're dealing with class types, "convertibility" is just conversion operators and constructors, so we're removing constructors from that list. If concurrent_node_map has a 1 argument implicit constructor from type U, unordered_node_map has that same implicit constructor from type U. Therefore there's no need to check for the constructors here, if we can get away with only checking conversion operators.

Neither of these methods work on GCC anyway, so I still don't know what to do.