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.

Edit from later: It turns out that my suggested MSVC solution doesn't actually work on MSVC afterall. But I did find a solution that works on GCC, using only the presence of the conversion operator. This doesn't work on MSVC or Clang.

nullptr !=
  &std::remove_reference<U>::type::operator concurrent_node_map<Key,
    T, Hash, KeyEqual, Allocator>

[joaquintides]

@k3DW it doesn't look like we're going to come up with a totally transparent solution right now, and Boost 1.91 is approaching, so, given that the reported issues looks important enough, I'd go with something like #334 (comment) and maybe file an issue to potentially continue investigating after the release.

[k3DW]

@k3DW it doesn't look like we're going to come up with a totally transparent solution right now, and Boost 1.91 is approaching, so, given that the reported issues looks important enough, I'd go with something like #334 (comment) and maybe file an issue to potentially continue investigating after the release.

I wasn't able to get to this in time for 1.91. I've updated this PR. I'll open a new issue. Even if we don't fix it soon, it'll be documented that this is a limitation.

[k3DW]

I'm not fully convinced that we need an issue to follow up. I don't know whether it matters. This change, as I have it right now, breaks certain constructors through conversion operators.

using flat_map = boost::unordered::unordered_flat_map<int, int>;
using c_flat_map = boost::unordered::concurrent_flat_map<int, int>;

struct converter
{
  operator c_flat_map();
};

static_assert(std::is_constructible_v<flat_map, converter>));

The use case above worked before this PR, and will be broken after it. But should we even try to support esoteric use cases like this? It's better to be more explicit in this case of a double conversion, from converter to c_flat_map, then to flat_map.

Note that constructing flat_map from a class that has a conversion operator to flat_map would still work.