review comments

Author: huixie90

libcxx/include/__flat_map/key_value_iterator.h

@@ -18,6 +18,7 @@
 #include <__iterator/product_iterator.h>
 #include <__memory/addressof.h>
 #include <__type_traits/conditional.h>
+#include <__utility/forward.h>
 #include <__utility/move.h>
 #include <__utility/pair.h>
 
@@ -175,17 +176,22 @@ template <class _Owner, class _KeyContainer, class _MappedContainer, bool _Const
 struct __product_iterator_traits<__key_value_iterator<_Owner, _KeyContainer, _MappedContainer, _Const>> {
   static constexpr size_t __size = 2;
 
-  template <size_t _Nth>
-  _LIBCPP_HIDE_FROM_ABI static auto
-  __get_iterator_element(__key_value_iterator<_Owner, _KeyContainer, _MappedContainer, _Const> __it)
+  template <size_t _Nth, class _Iter>
+  _LIBCPP_HIDE_FROM_ABI static decltype(auto) __get_iterator_element(_Iter&& __it)
     requires(_Nth <= 1)
   {
     if constexpr (_Nth == 0) {
-      return __it.__key_iter_;
+      return std::forward<_Iter>(__it).__key_iter_;
     } else {
-      return __it.__mapped_iter_;
+      return std::forward<_Iter>(__it).__mapped_iter_;
     }
   }
+
+  template <class _KeyIter, class _MappedIter>
+  _LIBCPP_HIDE_FROM_ABI static auto __make_product_iterator(_KeyIter&& __key_iter, _MappedIter&& __mapped_iter) {
+    return __key_value_iterator<_Owner, _KeyContainer, _MappedContainer, _Const>(
+        std::forward<_KeyIter>(__key_iter), std::forward<_MappedIter>(__mapped_iter));
+  }
 };
 
 _LIBCPP_END_NAMESPACE_STD

libcxx/include/__flat_map/utils.h

@@ -80,8 +80,6 @@ struct __flat_map_utils {
     return typename _Map::iterator(std::move(__key_it), std::move(__mapped_it));
   }
 
-  // TODO: We could optimize this, see
-  // https://github.com/llvm/llvm-project/issues/108624
   template <class _Map, class _InputIterator, class _Sentinel>
   _LIBCPP_HIDE_FROM_ABI static typename _Map::size_type
   __append(_Map& __map, _InputIterator __first, _Sentinel __last) {

libcxx/include/__iterator/product_iterator.h

@@ -21,7 +21,7 @@
 //   The number of underlying iterators inside the product iterator.
 //
 // - template <size_t _N>
-//   static auto Traits::__get_iterator_element(It __it)
+//   static decltype(auto) Traits::__get_iterator_element(It&& __it)
 //   Returns the _Nth iterator element of the given product iterator.
 
 #include <__config>
@@ -42,8 +42,11 @@ struct __product_iterator_traits;
 {
   static constexpr size_t __size = ...;
 
-  template <size_t _N>
-  static auto __get_iterator_element(_Iterator);
+  template <size_t _N, class _Iter>
+  static decltype(auto) __get_iterator_element(_Iter&&);
+
+  template <class... _Iters>
+  static _Iterator __make_product_iterator(_Iters&&...);
 };
 */
 

libcxx/include/__ranges/zip_view.h

@@ -473,15 +473,20 @@ inline constexpr auto zip = __zip::__fn{};
 } // namespace views
 } // namespace ranges
 
-template <class _Iter>
-  requires _Iter::__is_zip_view_iterator::value
-struct __product_iterator_traits<_Iter> {
-  static constexpr size_t __size = tuple_size<decltype(std::declval<_Iter>().__current_)>::value;
+template <class _Iterator>
+  requires _Iterator::__is_zip_view_iterator
+struct __product_iterator_traits<_Iterator> {
+  static constexpr size_t __size = tuple_size<decltype(std::declval<_Iterator>().__current_)>::value;
 
-  template <size_t _Nth>
+  template <size_t _Nth, class _Iter>
     requires(_Nth < __size)
-  _LIBCPP_HIDE_FROM_ABI static constexpr auto __get_iterator_element(_Iter __it) {
-    return std::get<_Nth>(__it.__current_);
+  _LIBCPP_HIDE_FROM_ABI static constexpr decltype(auto) __get_iterator_element(_Iter&& __it) {
+    return std::get<_Nth>(std::forward<_Iter>(__it).__current_);
+  }
+
+  template <class... _Iters>
+  _LIBCPP_HIDE_FROM_ABI static constexpr _Iterator __make_product_iterator(_Iters&&... __iters) {
+    return _Iterator(std::tuple(std::forward<_Iters>(__iters)...));
   }
 };
 

libcxx/test/benchmarks/containers/associative/associative_container_benchmarks.h

@@ -66,6 +66,8 @@ void associative_container_benchmarks(std::string container) {
 
   static constexpr bool is_ordered_container = requires(Container c, Key k) { c.lower_bound(k); };
 
+  static constexpr bool is_map_like = requires { typename Container::mapped_type; };
+
   // These benchmarks are structured to perform the operation being benchmarked
   // a small number of times at each iteration, in order to offset the cost of
   // PauseTiming() and ResumeTiming().
@@ -321,6 +323,47 @@ void associative_container_benchmarks(std::string container) {
     }
   });
 
+  if constexpr (is_map_like) {
+    bench("insert(iterator, iterator) (product_iterator from same type)", [=](auto& st) {
+      const std::size_t size = st.range(0);
+      std::vector<Value> in  = make_value_types(generate_unique_keys(size + (size / 10)));
+      std::sort(in.begin(), in.end(), [&](const auto& x, const auto& y) { return get_key(x) < get_key(y); });
+      Container source(in.begin(), in.end());
+
+      Container c;
+
+      for ([[maybe_unused]] auto _ : st) {
+        c.insert(source.begin(), source.end());
+        benchmark::DoNotOptimize(c);
+        benchmark::ClobberMemory();
+
+        st.PauseTiming();
+        c = Container();
+        st.ResumeTiming();
+      }
+    });
+
+    bench("insert(iterator, iterator) (product_iterator from zip_view)", [=](auto& st) {
+      const std::size_t size = st.range(0);
+      std::vector<Key> keys  = generate_unique_keys(size + (size / 10));
+      std::sort(keys.begin(), keys.end());
+      std::vector<typename Container::mapped_type> mapped(keys.size());
+
+      auto source = std::views::zip(keys, mapped);
+
+      Container c;
+
+      for ([[maybe_unused]] auto _ : st) {
+        c.insert(source.begin(), source.end());
+        benchmark::DoNotOptimize(c);
+        benchmark::ClobberMemory();
+
+        st.PauseTiming();
+        c = Container();
+        st.ResumeTiming();
+      }
+    });
+  }
   /////////////////////////
   // Erasure
   /////////////////////////

libcxx/test/benchmarks/containers/associative/flat_map.bench.cpp

@@ -27,54 +27,9 @@ struct support::adapt_operations<std::flat_map<K, V>> {
   static auto get_iterator(InsertionResult const& result) { return result.first; }
 };
 
-void product_iterator_benchmark_flat_map(benchmark::State& state) {
-  const std::size_t size = state.range(0);
-
-  using M = std::flat_map<int, int>;
-
-  const M source =
-      std::views::iota(0, static_cast<int>(size)) | std::views::transform([](int i) { return std::pair(i, i); }) |
-      std::ranges::to<std::flat_map<int, int>>();
-
-  for (auto _ : state) {
-    M m;
-    m.insert(std::sorted_unique, source.begin(), source.end());
-    benchmark::DoNotOptimize(m);
-    benchmark::ClobberMemory();
-  }
-}
-
-void product_iterator_benchmark_zip_view(benchmark::State& state) {
-  const std::size_t size = state.range(0);
-
-  using M = std::flat_map<int, int>;
-
-  const std::vector<int> keys   = std::views::iota(0, static_cast<int>(size)) | std::ranges::to<std::vector<int>>();
-  const std::vector<int> values = keys;
-
-  auto source = std::views::zip(keys, values);
-  for (auto _ : state) {
-    M m;
-    m.insert(std::sorted_unique, source.begin(), source.end());
-    benchmark::DoNotOptimize(m);
-    benchmark::ClobberMemory();
-  }
-}
-
 int main(int argc, char** argv) {
   support::associative_container_benchmarks<std::flat_map<int, int>>("std::flat_map<int, int>");
 
-  benchmark::RegisterBenchmark("flat_map::insert_product_iterator_flat_map", product_iterator_benchmark_flat_map)
-      ->Arg(32)
-      ->Arg(1024)
-      ->Arg(8192)
-      ->Arg(65536);
-  benchmark::RegisterBenchmark("flat_map::insert_product_iterator_zip", product_iterator_benchmark_zip_view)
-      ->Arg(32)
-      ->Arg(1024)
-      ->Arg(8192)
-      ->Arg(65536);
-
   benchmark::Initialize(&argc, argv);
   benchmark::RunSpecifiedBenchmarks();
   benchmark::Shutdown();

libcxx/test/libcxx/iterators/product_iterator.pass.cpp

@@ -0,0 +1,46 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+#include <ranges>
+#include <type_traits>
+
+#include "test_macros.h"
+#include "test_iterators.h"
+
+constexpr bool test() {
+  {
+    // Test that the __get_iterator_element can handle a non-copyable iterator
+    int Date[] = {1, 2, 3, 4};
+    cpp20_input_iterator<int*> iter(Date);
+    sentinel_wrapper<cpp20_input_iterator<int*>> sent{cpp20_input_iterator<int*>(Date + 4)};
+    std::ranges::subrange r1(std::move(iter), std::move(sent));
+    auto v  = std::views::zip(std::move(r1), std::views::iota(0, 4));
+    auto it = v.begin();
+
+    using Iter = decltype(it);
+
+    static_assert(!std::is_copy_constructible_v<Iter>);
+
+    static_assert(std::__product_iterator_traits<Iter>::__size == 2);
+    std::same_as<cpp20_input_iterator<int*>&> decltype(auto) it1 =
+        std::__product_iterator_traits<Iter>::__get_iterator_element<0>(it);
+
+    assert(*it1 == 1);
+  }
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+
+  return 0;
+}