Allow making sets and maps with move-only keys and/or values #4982
Conversation
|
We have a type (FacetTypeInfo) that we never want to copy (#4920), because it does the wrong thing. It's in a hash container however, so it breaks compile due to being move-only right now. Thoughts? |
|
Hm this looks like a clang-16 compiler issue; it builds with clang 17.0.6 |
When the key or value is move-only, then the set or map will be as well.
danakj
force-pushed
the
hash-copy-move
branch
from
2224417 to
4190314
Compare
Yeah it's a bug in std::pair comparison in clang-16. I def recall seeing this before. I added a workaround for clang-16 only. |
I am removing the 'should not be copied' property of that type in #4989 but maybe we'd want these things to work with move-only types in the future still? |
common/set_test.cpp
Outdated
| template <typename SetT> | ||
| class CopyableSetTest : public ::testing::Test {}; | ||
|
|
||
| using Types = | ||
| ::testing::Types<Set<int>, Set<int, 16>, Set<int, 128>, Set<TestData>, | ||
| Set<TestData, 16>, Set<MoveOnlyTestData>, | ||
| Set<MoveOnlyTestData, 16>>; | ||
| TYPED_TEST_SUITE(SetTest, Types); |
There was a problem hiding this comment.
What do you think about having a small / narrow test for move-only types and the restricted map API? I feel like that would let us avoid having the complexity of handling move-only types and skipping some of the tests in the suite below?
There was a problem hiding this comment.
Sure, I think that would be okay, done.
common/map_test.cpp
Outdated
| // Workaround for std::pair comparison deficiency in clang-16. | ||
| #if defined(__clang_major__) && __clang_major__ <= 16 | ||
| namespace std { | ||
| template <class T, class U, class V, class W> | ||
| requires(convertible_to<V, T> && convertible_to<W, U>) | ||
| inline auto operator==(pair<T&, U&> lhs, pair<V, W> rhs) -> bool { | ||
| return lhs.first == static_cast<T>(rhs.first) && | ||
| lhs.second == static_cast<U>(rhs.second); | ||
| } | ||
| } // namespace std | ||
| #endif |
There was a problem hiding this comment.
Should this be keyed on libc++ versions rather than Clang versions?
This is ... a pretty rough workaround...
There was a problem hiding this comment.
Good point, using _LIBCPP_VERSION now
Realizing I didn't make that clear -- yeah, I think supporting move only types is a good thing, so happy with the direction here. |
common/map_test.cpp
Outdated
| // Workaround for std::pair comparison deficiency in clang-16. | ||
| #if defined(__clang_major__) && __clang_major__ <= 16 | ||
| namespace std { | ||
| template <class T, class U, class V, class W> | ||
| requires(convertible_to<V, T> && convertible_to<W, U>) | ||
| inline auto operator==(pair<T&, U&> lhs, pair<V, W> rhs) -> bool { | ||
| return lhs.first == static_cast<T>(rhs.first) && | ||
| lhs.second == static_cast<U>(rhs.second); | ||
| } | ||
| } // namespace std | ||
| #endif |
There was a problem hiding this comment.
Good point, using _LIBCPP_VERSION now
common/set_test.cpp
Outdated
| template <typename SetT> | ||
| class CopyableSetTest : public ::testing::Test {}; | ||
|
|
||
| using Types = | ||
| ::testing::Types<Set<int>, Set<int, 16>, Set<int, 128>, Set<TestData>, | ||
| Set<TestData, 16>, Set<MoveOnlyTestData>, | ||
| Set<MoveOnlyTestData, 16>>; | ||
| TYPED_TEST_SUITE(SetTest, Types); |
There was a problem hiding this comment.
Sure, I think that would be okay, done.
When the key or value is move-only, then the set or map will be as well.