Fix and extend key extraction for unique map/set containers (#5050)

Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>

stl/inc/xhash

@@ -590,7 +590,7 @@ public:
     template <class... _Valtys>
     conditional_t<_Multi, iterator, pair<iterator, bool>> emplace(_Valtys&&... _Vals) {
         // try to insert value_type(_Vals...)
-        using _In_place_key_extractor = typename _Traits::template _In_place_key_extractor<_Remove_cvref_t<_Valtys>...>;
+        using _In_place_key_extractor = typename _Traits::template _In_place_key_extractor<_Valtys...>;
         if constexpr (_Multi) {
             _Check_max_size();
             _List_node_emplace_op2<_Alnode> _Newnode(_List._Getal(), _STD forward<_Valtys>(_Vals)...);
@@ -642,7 +642,7 @@ public:
 
     template <class... _Valtys>
     iterator emplace_hint(const_iterator _Hint, _Valtys&&... _Vals) { // try to insert value_type(_Vals...)
-        using _In_place_key_extractor = typename _Traits::template _In_place_key_extractor<_Remove_cvref_t<_Valtys>...>;
+        using _In_place_key_extractor = typename _Traits::template _In_place_key_extractor<_Valtys...>;
         if constexpr (_Multi) {
             _Check_max_size();
             _List_node_emplace_op2<_Alnode> _Newnode(_List._Getal(), _STD forward<_Valtys>(_Vals)...);

stl/inc/xmemory

@@ -2088,15 +2088,22 @@ _CXX17_DEPRECATE_TEMPORARY_BUFFER void return_temporary_buffer(_Ty* _Pbuf) {
 }
 #endif // _HAS_DEPRECATED_TEMPORARY_BUFFER
 
-// assumes _Args have already been _Remove_cvref_t'd
+// The key_type of an (unordered) associative container is cv-unqualified, and we can't bind const Key& to a
+// volatile glvalue. Also, Cpp17CopyInsertable and Cpp17MoveInsertable don't require value-preservation for
+// the construction from a volatile glvalue, so generally we can't perform this optimization for them.
+// See N4993 [container.alloc.reqmts]/2.3, /2.4.
+template <class _Ty>
+using _Remove_const_ref_t = remove_const_t<remove_reference_t<_Ty>>;
+
+// assumes _Args have already been _Remove_const_ref_t'd
 template <class _Key, class... _Args>
-struct _In_place_key_extract_set {
+struct _In_place_key_extract_set_impl {
     // by default we can't extract the key in the emplace family and must construct a node we might not use
     static constexpr bool _Extractable = false;
 };
 
 template <class _Key>
-struct _In_place_key_extract_set<_Key, _Key> {
+struct _In_place_key_extract_set_impl<_Key, _Key> {
     // we can extract the key in emplace if the emplaced type is identical to the key type
     static constexpr bool _Extractable = true;
     static const _Key& _Extract(const _Key& _Val) noexcept {
@@ -2104,15 +2111,18 @@ struct _In_place_key_extract_set<_Key, _Key> {
     }
 };
 
-// assumes _Args have already been _Remove_cvref_t'd
+template <class... _Valtys>
+using _In_place_key_extract_set = _In_place_key_extract_set_impl<_Remove_const_ref_t<_Valtys>...>;
+
+// assumes _Args have already been _Remove_const_ref_t'd
 template <class _Key, class... _Args>
-struct _In_place_key_extract_map {
+struct _In_place_key_extract_map_impl {
     // by default we can't extract the key in the emplace family and must construct a node we might not use
     static constexpr bool _Extractable = false;
 };
 
 template <class _Key, class _Second>
-struct _In_place_key_extract_map<_Key, _Key, _Second> {
+struct _In_place_key_extract_map_impl<_Key, _Key, _Second> {
     // if we would call the pair(key, value) constructor family, we can use the first parameter as the key
     static constexpr bool _Extractable = true;
     static const _Key& _Extract(const _Key& _Val, const _Second&) noexcept {
@@ -2121,14 +2131,49 @@ struct _In_place_key_extract_map<_Key, _Key, _Second> {
 };
 
 template <class _Key, class _First, class _Second>
-struct _In_place_key_extract_map<_Key, pair<_First, _Second>> {
+struct _In_place_key_extract_map_impl<_Key, pair<_First, _Second>> {
     // if we would call the pair(pair<other, other>) constructor family, we can use the pair.first member as the key
-    static constexpr bool _Extractable = is_same_v<_Key, _Remove_cvref_t<_First>>;
-    static const _Key& _Extract(const pair<_First, _Second>& _Val) {
+    static constexpr bool _Extractable = is_same_v<_Key, _Remove_const_ref_t<_First>>;
+    static const _Key& _Extract(const pair<_First, _Second>& _Val) noexcept {
         return _Val.first;
     }
 };
 
+#if _HAS_CXX23
+// if we would call the pair(pair-like) constructor family and the argument is not a subrange,
+// we can use get<0>(pair-like) as the key
+
+template <class _Key, class _Elem>
+struct _In_place_key_extract_map_impl<_Key, array<_Elem, 2>> {
+    static constexpr bool _Extractable = is_same_v<_Key, remove_const_t<_Elem>>;
+    static const _Key& _Extract(const array<_Elem, 2>& _Val) noexcept {
+        return _Val[0];
+    }
+};
+
+template <class _Key, class _First, class _Second>
+struct _In_place_key_extract_map_impl<_Key, tuple<_First, _Second>> {
+    static constexpr bool _Extractable = is_same_v<_Key, _Remove_const_ref_t<_First>>;
+    static const _Key& _Extract(const tuple<_First, _Second>& _Val) noexcept {
+        return _STD get<0>(_Val);
+    }
+};
+#endif // _HAS_CXX23
+
+template <class _Key, class _First, class... _RestTypes>
+struct _In_place_key_extract_map_impl<_Key, piecewise_construct_t, tuple<_First>, tuple<_RestTypes...>> {
+    // if we would call the piecewise_construct_t constructor and the first argument is a 1-tuple,
+    // we can use get<0>(first_tuple) as the key
+    static constexpr bool _Extractable = is_same_v<_Key, _Remove_const_ref_t<_First>>;
+    static const _Key& _Extract(
+        const piecewise_construct_t&, const tuple<_First>& _Tup_val, const tuple<_RestTypes...>&) noexcept {
+        return _STD get<0>(_Tup_val);
+    }
+};
+
+template <class... _Valtys>
+using _In_place_key_extract_map = _In_place_key_extract_map_impl<_Remove_const_ref_t<_Valtys>...>;
+
 #pragma warning(push)
 #pragma warning(disable : 4624) // '%s': destructor was implicitly defined as deleted
 template <class _Ty>

stl/inc/xtree

@@ -998,7 +998,7 @@ private:
 protected:
     template <class... _Valtys>
     pair<_Nodeptr, bool> _Emplace(_Valtys&&... _Vals) {
-        using _In_place_key_extractor = typename _Traits::template _In_place_key_extractor<_Remove_cvref_t<_Valtys>...>;
+        using _In_place_key_extractor = typename _Traits::template _In_place_key_extractor<_Valtys...>;
         const auto _Scary             = _Get_scary();
         _Tree_find_result<_Nodeptr> _Loc;
         _Nodeptr _Inserted;
@@ -1042,7 +1042,7 @@ public:
 protected:
     template <class... _Valtys>
     _Nodeptr _Emplace_hint(const _Nodeptr _Hint, _Valtys&&... _Vals) {
-        using _In_place_key_extractor = typename _Traits::template _In_place_key_extractor<_Remove_cvref_t<_Valtys>...>;
+        using _In_place_key_extractor = typename _Traits::template _In_place_key_extractor<_Valtys...>;
         const auto _Scary             = _Get_scary();
         _Tree_find_hint_result<_Nodeptr> _Loc;
         _Nodeptr _Inserted;

tests/std/tests/VSO_0131167_associative_emplacement_allocations/test.cpp

@@ -1,9 +1,14 @@
 // Copyright (c) Microsoft Corporation.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
+#include <array>
 #include <cassert>
 #include <functional>
+#include <map>
 #include <memory>
+#include <set>
+#include <tuple>
+#include <type_traits>
 #include <unordered_map>
 #include <unordered_set>
 #include <utility>
@@ -142,23 +147,282 @@ void test_emplaces(FirstValue& val1, Values&... valn) {
     }
 }
 
-int main() {
-    // This tests that unordered_(map|set) detect cases they can avoid allocating.
-    // It isn't strictly required by the standard, but we think it should be applicable to good implementations.
+template <class SetContainer>
+void test_emplace_for_unique_set() {
     int lvalueInt{42};
-    test_emplaces<unordered_set<int, hash<int>, equal_to<>, TestAlloc<int>>>(lvalueInt);
-    using MapType = unordered_map<int, int, hash<int>, equal_to<>, TestAlloc<pair<const int, int>>>;
-    test_emplaces<MapType>(lvalueInt, lvalueInt);
+    test_emplaces<SetContainer>(lvalueInt);
+}
+
+template <class MapContainer>
+void test_emplace_for_unique_map() {
+    int lvalueInt{42};
+    test_emplaces<MapContainer>(lvalueInt, lvalueInt);
+
     pair<const int, int> lvalueConstPair{42, 65};
     pair<int, int> lvaluePair{42, 65};
     pair<const int&, int> lvalueConstRefPair{lvalueInt, 65};
     pair<int&, int> lvalueRefPair{lvalueInt, 65};
     pair<const int&&, int> lvalueConstRefRefPair{move(lvalueInt), 65};
     pair<int&&, int> lvalueRefRefPair{move(lvalueInt), 65};
-    test_emplaces<MapType>(lvalueConstPair);
-    test_emplaces<MapType>(lvaluePair);
-    test_emplaces<MapType>(lvalueConstRefPair);
-    test_emplaces<MapType>(lvalueRefPair);
-    test_emplaces<MapType>(lvalueConstRefRefPair);
-    test_emplaces<MapType>(lvalueRefRefPair);
+    test_emplaces<MapContainer>(lvalueConstPair);
+    test_emplaces<MapContainer>(lvaluePair);
+    test_emplaces<MapContainer>(lvalueConstRefPair);
+    test_emplaces<MapContainer>(lvalueRefPair);
+    test_emplaces<MapContainer>(lvalueConstRefRefPair);
+    test_emplaces<MapContainer>(lvalueRefRefPair);
+
+#if _HAS_CXX23
+    tuple<const int, int> lvalueConstTuple{42, 65};
+    tuple<int, int> lvalueTuple{42, 65};
+    tuple<const int&, int> lvalueConstRefTuple{lvalueInt, 65};
+    tuple<int&, int> lvalueRefTuple{lvalueInt, 65};
+    tuple<const int&&, int> lvalueConstRefRefTuple{move(lvalueInt), 65};
+    tuple<int&&, int> lvalueRefRefTuple{move(lvalueInt), 65};
+    test_emplaces<MapContainer>(lvalueConstTuple);
+    test_emplaces<MapContainer>(lvalueTuple);
+    test_emplaces<MapContainer>(lvalueConstRefTuple);
+    test_emplaces<MapContainer>(lvalueRefTuple);
+    test_emplaces<MapContainer>(lvalueConstRefRefTuple);
+    test_emplaces<MapContainer>(lvalueRefRefTuple);
+
+    array<int, 2> arr{42, 65};
+    array<const int, 2> constArr{42, 65};
+    test_emplaces<MapContainer>(arr);
+    test_emplaces<MapContainer>(constArr);
+#endif // _HAS_CXX23
+
+    tuple<int> tupleIntSixtyFive{65};
+
+    tuple<const int> lvalueConstOneTuple{42};
+    tuple<int> lvalueOneTuple{42};
+    tuple<const int&> lvalueConstRefOneTuple{lvalueInt};
+    tuple<int&> lvalueRefOneTuple{lvalueInt};
+    test_emplaces<MapContainer>(piecewise_construct, lvalueConstOneTuple, tupleIntSixtyFive);
+    test_emplaces<MapContainer>(piecewise_construct, lvalueOneTuple, tupleIntSixtyFive);
+    test_emplaces<MapContainer>(piecewise_construct, lvalueConstRefOneTuple, tupleIntSixtyFive);
+    test_emplaces<MapContainer>(piecewise_construct, lvalueRefOneTuple, tupleIntSixtyFive);
+}
+
+// also test that the optimization strategy does not mishandle volatile arguments
+template <class SetContainer>
+void test_volatile_arguments_for_unique_set() {
+    using Key = typename SetContainer::value_type;
+
+    SetContainer s;
+
+    volatile Key x         = 0;
+    const volatile Key& cx = x;
+
+    s.emplace(x);
+    x = 1;
+    s.emplace(move(x));
+    x = 2;
+    s.emplace(cx);
+    x = 3;
+    s.emplace(move(cx));
+
+    x = 4;
+    s.emplace_hint(s.end(), x);
+    x = 5;
+    s.emplace_hint(s.end(), move(x));
+    x = 6;
+    s.emplace_hint(s.end(), cx);
+    x = 7;
+    s.emplace_hint(s.end(), move(cx));
+
+    assert((s == SetContainer{0, 1, 2, 3, 4, 5, 6, 7}));
+}
+
+template <class MapContainer, class PairLike>
+void test_pair_like_volatile_for_unique_map() {
+    using First  = tuple_element_t<0, PairLike>;
+    using Second = tuple_element_t<1, PairLike>;
+
+    MapContainer m;
+
+    volatile remove_cv_t<remove_reference_t<First>> x = 0;
+    {
+        PairLike p{static_cast<First>(x), Second{}};
+        m.emplace(p);
+    }
+
+    x = 1;
+    {
+        PairLike p{static_cast<First>(x), Second{}};
+        m.emplace(move(p));
+    }
+
+    x = 2;
+    {
+        PairLike p{static_cast<First>(x), Second{}};
+        const auto& cp = p;
+        m.emplace(cp);
+    }
+
+    x = 3;
+    {
+        PairLike p{static_cast<First>(x), Second{}};
+        const auto& cp = p;
+        m.emplace(move(cp));
+    }
+
+    x = 4;
+    {
+        PairLike p{static_cast<First>(x), Second{}};
+        m.emplace_hint(m.end(), p);
+    }
+
+    x = 5;
+    {
+        PairLike p{static_cast<First>(x), Second{}};
+        m.emplace_hint(m.end(), move(p));
+    }
+
+    x = 6;
+    {
+        PairLike p{static_cast<First>(x), Second{}};
+        const auto& cp = p;
+        m.emplace_hint(m.end(), cp);
+    }
+
+    x = 7;
+    {
+        PairLike p{static_cast<First>(x), Second{}};
+        const auto& cp = p;
+        m.emplace_hint(m.end(), move(cp));
+    }
+
+    assert((m == MapContainer{{0, {}}, {1, {}}, {2, {}}, {3, {}}, {4, {}}, {5, {}}, {6, {}}, {7, {}}}));
+}
+
+template <class MapContainer, class Tuple>
+void test_piecewise_volatile_for_unique_map() {
+    using First = tuple_element_t<0, Tuple>;
+
+    MapContainer m;
+
+    volatile remove_cv_t<remove_reference_t<First>> x = 0;
+    {
+        Tuple tp{static_cast<First>(x)};
+        m.emplace(piecewise_construct, tp, tuple<>{});
+    }
+
+    x = 1;
+    {
+        Tuple tp{static_cast<First>(x)};
+        m.emplace(piecewise_construct, move(tp), tuple<>{});
+    }
+
+    x = 2;
+    {
+        Tuple tp{static_cast<First>(x)};
+        const auto& ctp = tp;
+        m.emplace(piecewise_construct, ctp, tuple<>{});
+    }
+
+    x = 3;
+    {
+        Tuple tp{static_cast<First>(x)};
+        const auto& ctp = tp;
+        m.emplace(piecewise_construct, move(ctp), tuple<>{});
+    }
+
+    x = 4;
+    {
+        Tuple tp{static_cast<First>(x)};
+        m.emplace_hint(m.end(), piecewise_construct, tp, tuple<>{});
+    }
+
+    x = 5;
+    {
+        Tuple tp{static_cast<First>(x)};
+        m.emplace_hint(m.end(), piecewise_construct, move(tp), tuple<>{});
+    }
+
+    x = 6;
+    {
+        Tuple tp{static_cast<First>(x)};
+        const auto& ctp = tp;
+        m.emplace_hint(m.end(), piecewise_construct, ctp, tuple<>{});
+    }
+
+    x = 7;
+    {
+        Tuple tp{static_cast<First>(x)};
+        const auto& ctp = tp;
+        m.emplace_hint(m.end(), piecewise_construct, move(ctp), tuple<>{});
+    }
+
+    assert((m == MapContainer{{0, {}}, {1, {}}, {2, {}}, {3, {}}, {4, {}}, {5, {}}, {6, {}}, {7, {}}}));
+}
+
+template <class MapContainer>
+void test_volatile_arguments_for_unique_map() {
+    using Key    = typename MapContainer::key_type;
+    using Mapped = typename MapContainer::mapped_type;
+
+    {
+        volatile Key x         = 0;
+        const volatile Key& cx = x;
+
+        MapContainer m;
+
+        m.emplace(x, Mapped{});
+        x = 1;
+        m.emplace(move(x), Mapped{});
+        x = 2;
+        m.emplace(cx, Mapped{});
+        x = 3;
+        m.emplace(move(cx), Mapped{});
+
+        x = 4;
+        m.emplace_hint(m.end(), x, Mapped{});
+        x = 5;
+        m.emplace_hint(m.end(), move(x), Mapped{});
+        x = 6;
+        m.emplace_hint(m.end(), cx, Mapped{});
+        x = 7;
+        m.emplace_hint(m.end(), move(cx), Mapped{});
+
+        assert((m == MapContainer{{0, {}}, {1, {}}, {2, {}}, {3, {}}, {4, {}}, {5, {}}, {6, {}}, {7, {}}}));
+    }
+
+    test_pair_like_volatile_for_unique_map<MapContainer, pair<volatile Key, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, pair<const volatile Key, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, pair<volatile Key&, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, pair<const volatile Key&, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, pair<volatile Key&&, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, pair<const volatile Key&&, Mapped>>();
+#if _HAS_CXX23
+    test_pair_like_volatile_for_unique_map<MapContainer, tuple<volatile Key, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, tuple<const volatile Key, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, tuple<volatile Key&, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, tuple<const volatile Key&, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, tuple<volatile Key&&, Mapped>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, tuple<const volatile Key&&, Mapped>>();
+
+    test_pair_like_volatile_for_unique_map<MapContainer, array<volatile Key, 2>>();
+    test_pair_like_volatile_for_unique_map<MapContainer, array<const volatile Key, 2>>();
+#endif // _HAS_CXX23
+
+    test_piecewise_volatile_for_unique_map<MapContainer, tuple<volatile Key>>();
+    test_piecewise_volatile_for_unique_map<MapContainer, tuple<const volatile Key>>();
+    test_piecewise_volatile_for_unique_map<MapContainer, tuple<volatile Key&>>();
+    test_piecewise_volatile_for_unique_map<MapContainer, tuple<const volatile Key&>>();
+}
+
+int main() {
+    // This tests that unordered_(map|set) detect cases they can avoid allocating.
+    // It isn't strictly required by the standard, but we think it should be applicable to good implementations.
+    test_emplace_for_unique_set<set<int, less<>, TestAlloc<int>>>();
+    test_emplace_for_unique_set<unordered_set<int, hash<int>, equal_to<>, TestAlloc<int>>>();
+
+    test_emplace_for_unique_map<map<int, int, less<>, TestAlloc<pair<const int, int>>>>();
+    test_emplace_for_unique_map<unordered_map<int, int, hash<int>, equal_to<>, TestAlloc<pair<const int, int>>>>();
+
+    test_volatile_arguments_for_unique_set<set<int>>();
+    test_volatile_arguments_for_unique_set<unordered_set<int>>();
+
+    test_volatile_arguments_for_unique_map<map<int, long>>();
+    test_volatile_arguments_for_unique_map<unordered_map<int, long>>();
 }