Skip to content

[libc++] Move out flat_map::iterator (for reusing it in flat_multimap) #117445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 9, 2024
Merged

[libc++] Move out flat_map::iterator (for reusing it in flat_multimap) #117445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e95ebf2
[libc++] Move out (for reusing it in flat_multimap)
Nov 23, 2024
a1ae892
review comments
Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions libcxx/include/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ set(files
__filesystem/space_info.h
__filesystem/u8path.h
__flat_map/flat_map.h
__flat_map/key_value_iterator.h
__flat_map/sorted_unique.h
__format/buffer.h
__format/concepts.h
Expand Down
128 changes: 5 additions & 123 deletions libcxx/include/__flat_map/flat_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@
#include <__algorithm/remove_if.h>
#include <__assert>
#include <__compare/synth_three_way.h>
#include <__concepts/convertible_to.h>
#include <__concepts/swappable.h>
#include <__config>
#include <__cstddef/byte.h>
#include <__cstddef/ptrdiff_t.h>
#include <__flat_map/key_value_iterator.h>
#include <__flat_map/sorted_unique.h>
#include <__functional/invoke.h>
#include <__functional/is_transparent.h>
Expand All @@ -38,7 +38,6 @@
#include <__iterator/next.h>
#include <__iterator/ranges_iterator_traits.h>
#include <__iterator/reverse_iterator.h>
#include <__memory/addressof.h>
#include <__memory/allocator_traits.h>
#include <__memory/uses_allocator.h>
#include <__memory/uses_allocator_construction.h>
Expand All @@ -57,8 +56,8 @@
#include <__type_traits/is_allocator.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_same.h>
#include <__type_traits/maybe_const.h>
#include <__utility/exception_guard.h>
#include <__utility/move.h>
#include <__utility/pair.h>
#include <__utility/scope_guard.h>
#include <__vector/vector.h>
Expand All @@ -82,9 +81,6 @@ template <class _Key,
class _KeyContainer = vector<_Key>,
class _MappedContainer = vector<_Tp>>
class flat_map {
template <bool _Const>
struct __iterator;

template <class, class, class, class, class>
friend class flat_map;

Expand All @@ -93,6 +89,9 @@ class flat_map {
static_assert(!is_same_v<_KeyContainer, std::vector<bool>>, "vector<bool> is not a sequence container");
static_assert(!is_same_v<_MappedContainer, std::vector<bool>>, "vector<bool> is not a sequence container");

template <bool _Const>
using __iterator = __key_value_iterator<flat_map, _KeyContainer, _MappedContainer, _Const>;

public:
// types
using key_type = _Key;
Expand Down Expand Up @@ -134,123 +133,6 @@ class flat_map {

_LIBCPP_HIDE_FROM_ABI static constexpr bool __is_compare_transparent = __is_transparent_v<_Compare, _Compare>;

template <bool _Const>
struct __iterator {
private:
using __key_iterator = ranges::iterator_t<const key_container_type>;
using __mapped_iterator = ranges::iterator_t<__maybe_const<_Const, mapped_container_type>>;
using __reference = pair<iter_reference_t<__key_iterator>, iter_reference_t<__mapped_iterator>>;

struct __arrow_proxy {
__reference __ref_;
_LIBCPP_HIDE_FROM_ABI __reference* operator->() { return std::addressof(__ref_); }
};

__key_iterator __key_iter_;
__mapped_iterator __mapped_iter_;

friend flat_map;

public:
using iterator_concept = random_access_iterator_tag;
// `flat_map::iterator` only satisfy "Cpp17InputIterator" named requirements, because
// its `reference` is not a reference type.
// However, to avoid surprising runtime behaviour when it is used with the
// Cpp17 algorithms or operations, iterator_category is set to random_access_iterator_tag.
using iterator_category = random_access_iterator_tag;
using value_type = flat_map::value_type;
using difference_type = flat_map::difference_type;

_LIBCPP_HIDE_FROM_ABI __iterator() = default;

_LIBCPP_HIDE_FROM_ABI __iterator(__iterator<!_Const> __i)
requires _Const && convertible_to<ranges::iterator_t<key_container_type>, __key_iterator> &&
convertible_to<ranges::iterator_t<mapped_container_type>, __mapped_iterator>
: __key_iter_(std::move(__i.__key_iter_)), __mapped_iter_(std::move(__i.__mapped_iter_)) {}

_LIBCPP_HIDE_FROM_ABI __iterator(__key_iterator __key_iter, __mapped_iterator __mapped_iter)
: __key_iter_(std::move(__key_iter)), __mapped_iter_(std::move(__mapped_iter)) {}

_LIBCPP_HIDE_FROM_ABI __reference operator*() const { return __reference(*__key_iter_, *__mapped_iter_); }
_LIBCPP_HIDE_FROM_ABI __arrow_proxy operator->() const { return __arrow_proxy{**this}; }

_LIBCPP_HIDE_FROM_ABI __iterator& operator++() {
++__key_iter_;
++__mapped_iter_;
return *this;
}

_LIBCPP_HIDE_FROM_ABI __iterator operator++(int) {
__iterator __tmp(*this);
++*this;
return __tmp;
}

_LIBCPP_HIDE_FROM_ABI __iterator& operator--() {
--__key_iter_;
--__mapped_iter_;
return *this;
}

_LIBCPP_HIDE_FROM_ABI __iterator operator--(int) {
__iterator __tmp(*this);
--*this;
return __tmp;
}

_LIBCPP_HIDE_FROM_ABI __iterator& operator+=(difference_type __x) {
__key_iter_ += __x;
__mapped_iter_ += __x;
return *this;
}

_LIBCPP_HIDE_FROM_ABI __iterator& operator-=(difference_type __x) {
__key_iter_ -= __x;
__mapped_iter_ -= __x;
return *this;
}

_LIBCPP_HIDE_FROM_ABI __reference operator[](difference_type __n) const { return *(*this + __n); }

_LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const __iterator& __x, const __iterator& __y) {
return __x.__key_iter_ == __y.__key_iter_;
}

_LIBCPP_HIDE_FROM_ABI friend bool operator<(const __iterator& __x, const __iterator& __y) {
return __x.__key_iter_ < __y.__key_iter_;
}

_LIBCPP_HIDE_FROM_ABI friend bool operator>(const __iterator& __x, const __iterator& __y) { return __y < __x; }

_LIBCPP_HIDE_FROM_ABI friend bool operator<=(const __iterator& __x, const __iterator& __y) { return !(__y < __x); }

_LIBCPP_HIDE_FROM_ABI friend bool operator>=(const __iterator& __x, const __iterator& __y) { return !(__x < __y); }

_LIBCPP_HIDE_FROM_ABI friend auto operator<=>(const __iterator& __x, const __iterator& __y)
requires three_way_comparable<__key_iterator>
{
return __x.__key_iter_ <=> __y.__key_iter_;
}

_LIBCPP_HIDE_FROM_ABI friend __iterator operator+(const __iterator& __i, difference_type __n) {
auto __tmp = __i;
__tmp += __n;
return __tmp;
}

_LIBCPP_HIDE_FROM_ABI friend __iterator operator+(difference_type __n, const __iterator& __i) { return __i + __n; }

_LIBCPP_HIDE_FROM_ABI friend __iterator operator-(const __iterator& __i, difference_type __n) {
auto __tmp = __i;
__tmp -= __n;
return __tmp;
}

_LIBCPP_HIDE_FROM_ABI friend difference_type operator-(const __iterator& __x, const __iterator& __y) {
return difference_type(__x.__key_iter_ - __y.__key_iter_);
}
};

public:
// [flat.map.cons], construct/copy/destroy
_LIBCPP_HIDE_FROM_ABI flat_map() noexcept(
Expand Down
177 changes: 177 additions & 0 deletions libcxx/include/__flat_map/key_value_iterator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
// -*- C++ -*-
//===----------------------------------------------------------------------===//
//
// 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
//
//===----------------------------------------------------------------------===//

#ifndef _LIBCPP___FLAT_MAP_KEY_VALUE_ITERATOR_H
#define _LIBCPP___FLAT_MAP_KEY_VALUE_ITERATOR_H

#include <__compare/three_way_comparable.h>
#include <__concepts/convertible_to.h>
#include <__config>
#include <__iterator/iterator_traits.h>
#include <__memory/addressof.h>
#include <__ranges/access.h>
#include <__type_traits/conditional.h>
#include <__type_traits/maybe_const.h>
#include <__utility/move.h>
#include <__utility/pair.h>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
#endif

_LIBCPP_PUSH_MACROS
#include <__undef_macros>

#if _LIBCPP_STD_VER >= 23

_LIBCPP_BEGIN_NAMESPACE_STD

/**
* __key_value_iterator is a proxy iterator which zips the underlying
* _KeyContainer::iterator and the underlying _MappedContainer::iterator.
* The two underlying iterators will be incremented/decremented together.
* And the reference is a pair of the const key reference and the value reference.
*/
template <class _Owner, class _KeyContainer, class _MappedContainer, bool _Const>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intended that two different flat_(multi)map specializations never have the same iterator types (i.e. the design is completely non-SCARY)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a requirement to provide SCARY iterators? I think there were some types that were required to provide SCARY iterators since C++11, but my memory is failing me. I'm not certain if there is some library-wide requirement about this?

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a requirement to provide SCARY iterators? I think there were some types that were required to provide SCARY iterators since C++11, but my memory is failing me. I'm not certain if there is some library-wide requirement about this?

No. N2980 proposed requiring SCARY, but it was effectively rejected (per LWG1242). Currently SCARY-ness is totally part of quality of implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had discussion with @ldionne, we definitely don't want to allow flat_map and flat_multimap's iterator to be interchangeable. Supporting SCARY iterators would allow erroneous programs which can be easily caught at compile time otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Or at least, that's our current thinking. We're open to being persuaded otherwise but our thinking was that if a user has the bad luck of unintendedly mixing up flat_map::iterator with flat_multimap::iterator, they'd rather know that at compile-time. IOW, not providing SCARY iterators seems like a feature more than a QOI deficiency in this situation. As I said, that's just our current thinking and it can evolve.

Copy link
Member Author

@huixie90 huixie90 Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldionne We could be a little bit SCARY where we don' allow the mix of flat_map and flat_multimap. But we can allow the mix of flat_map<int, int, std::less<int>>::iterator with flat_map<int, int, std::greater<int>>::iterator. (The benefit is less number of template instantiations but one could also argue this is a bug rather than a feature). That is what we did for map
https://godbolt.org/z/57zTGoGd3

To achieve this, we could do something like

template <class _Tag, 
                  class _KeyContainer, 
                  class _MappedContainer,
                  class _Reference,
                  class _ConstReference,
                  class _ValueType,
                  class _DifferenceType,
                  bool _Const>
struct __key_value_iterator

where _Tag can be __flat_map_tag or __flat_multimap_tag

However, I am not a big fan of SCARY iterators. I think it is a Non-QoI rather than a QoI. The mix of those iterators are extremely likely a bug in the user code where we could easily detect at compile time. SCARY iterators simply allow the erroneous user code which against making C++ a safe language initiative.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In https://devblogs.microsoft.com/cppblog/what-are-scary-iterators/, they make a point that e.g. different allocators should probably not result in different iterators. That makes some sense to me, although I guess it depends what the pointer type is. But here it seems like the amount of stuff we need to cary around in __key_value_iterator is probably not worth it.

struct __key_value_iterator {
private:
using __key_iterator = ranges::iterator_t<const _KeyContainer>;
using __mapped_iterator = ranges::iterator_t<__maybe_const<_Const, _MappedContainer>>;
using __reference = _If<_Const, typename _Owner::const_reference, typename _Owner::reference>;

struct __arrow_proxy {
__reference __ref_;
_LIBCPP_HIDE_FROM_ABI __reference* operator->() { return std::addressof(__ref_); }
};

__key_iterator __key_iter_;
__mapped_iterator __mapped_iter_;

friend _Owner;

template <class, class, class, bool>
friend struct __key_value_iterator;

public:
using iterator_concept = random_access_iterator_tag;
// `__key_value_iterator` only satisfy "Cpp17InputIterator" named requirements, because
// its `reference` is not a reference type.
// However, to avoid surprising runtime behaviour when it is used with the
// Cpp17 algorithms or operations, iterator_category is set to random_access_iterator_tag.
using iterator_category = random_access_iterator_tag;
using value_type = typename _Owner::value_type;
using difference_type = typename _Owner::difference_type;

_LIBCPP_HIDE_FROM_ABI __key_value_iterator() = default;

_LIBCPP_HIDE_FROM_ABI __key_value_iterator(__key_value_iterator<_Owner, _KeyContainer, _MappedContainer, !_Const> __i)
requires _Const && convertible_to<ranges::iterator_t<_KeyContainer>, __key_iterator> &&
convertible_to<ranges::iterator_t<_MappedContainer>, __mapped_iterator>
: __key_iter_(std::move(__i.__key_iter_)), __mapped_iter_(std::move(__i.__mapped_iter_)) {}

_LIBCPP_HIDE_FROM_ABI __key_value_iterator(__key_iterator __key_iter, __mapped_iterator __mapped_iter)
: __key_iter_(std::move(__key_iter)), __mapped_iter_(std::move(__mapped_iter)) {}

_LIBCPP_HIDE_FROM_ABI __reference operator*() const { return __reference(*__key_iter_, *__mapped_iter_); }
_LIBCPP_HIDE_FROM_ABI __arrow_proxy operator->() const { return __arrow_proxy{**this}; }

_LIBCPP_HIDE_FROM_ABI __key_value_iterator& operator++() {
++__key_iter_;
++__mapped_iter_;
return *this;
}

_LIBCPP_HIDE_FROM_ABI __key_value_iterator operator++(int) {
__key_value_iterator __tmp(*this);
++*this;
return __tmp;
}

_LIBCPP_HIDE_FROM_ABI __key_value_iterator& operator--() {
--__key_iter_;
--__mapped_iter_;
return *this;
}

_LIBCPP_HIDE_FROM_ABI __key_value_iterator operator--(int) {
__key_value_iterator __tmp(*this);
--*this;
return __tmp;
}

_LIBCPP_HIDE_FROM_ABI __key_value_iterator& operator+=(difference_type __x) {
__key_iter_ += __x;
__mapped_iter_ += __x;
return *this;
}

_LIBCPP_HIDE_FROM_ABI __key_value_iterator& operator-=(difference_type __x) {
__key_iter_ -= __x;
__mapped_iter_ -= __x;
return *this;
}

_LIBCPP_HIDE_FROM_ABI __reference operator[](difference_type __n) const { return *(*this + __n); }

_LIBCPP_HIDE_FROM_ABI friend constexpr bool
operator==(const __key_value_iterator& __x, const __key_value_iterator& __y) {
return __x.__key_iter_ == __y.__key_iter_;
}

_LIBCPP_HIDE_FROM_ABI friend bool operator<(const __key_value_iterator& __x, const __key_value_iterator& __y) {
return __x.__key_iter_ < __y.__key_iter_;
}

_LIBCPP_HIDE_FROM_ABI friend bool operator>(const __key_value_iterator& __x, const __key_value_iterator& __y) {
return __y < __x;
}

_LIBCPP_HIDE_FROM_ABI friend bool operator<=(const __key_value_iterator& __x, const __key_value_iterator& __y) {
return !(__y < __x);
}

_LIBCPP_HIDE_FROM_ABI friend bool operator>=(const __key_value_iterator& __x, const __key_value_iterator& __y) {
return !(__x < __y);
}

_LIBCPP_HIDE_FROM_ABI friend auto operator<=>(const __key_value_iterator& __x, const __key_value_iterator& __y)
requires three_way_comparable<__key_iterator>
{
return __x.__key_iter_ <=> __y.__key_iter_;
}

_LIBCPP_HIDE_FROM_ABI friend __key_value_iterator operator+(const __key_value_iterator& __i, difference_type __n) {
auto __tmp = __i;
__tmp += __n;
return __tmp;
}

_LIBCPP_HIDE_FROM_ABI friend __key_value_iterator operator+(difference_type __n, const __key_value_iterator& __i) {
return __i + __n;
}

_LIBCPP_HIDE_FROM_ABI friend __key_value_iterator operator-(const __key_value_iterator& __i, difference_type __n) {
auto __tmp = __i;
__tmp -= __n;
return __tmp;
}

_LIBCPP_HIDE_FROM_ABI friend difference_type
operator-(const __key_value_iterator& __x, const __key_value_iterator& __y) {
return difference_type(__x.__key_iter_ - __y.__key_iter_);
}
};

_LIBCPP_END_NAMESPACE_STD

#endif // _LIBCPP_STD_VER >= 23

_LIBCPP_POP_MACROS

#endif // _LIBCPP___FLAT_MAP_KEY_VALUE_ITERATOR_H
1 change: 1 addition & 0 deletions libcxx/include/module.modulemap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1230,6 +1230,7 @@ module std [system] {

module flat_map {
module flat_map { header "__flat_map/flat_map.h" }
module key_value_iterator { header "__flat_map/key_value_iterator.h" }
module sorted_unique { header "__flat_map/sorted_unique.h" }

header "flat_map"
Expand Down
1 change: 1 addition & 0 deletions ...est/std/containers/container.adaptors/flat.map/flat.map.cons/iter_iter_stability.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <flat_map>
#include <random>
#include <map>
#include <vector>

#include "test_macros.h"

Expand Down
2 changes: 1 addition & 1 deletion ...t/std/containers/container.adaptors/flat.map/flat.map.iterators/reverse_iterator.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#include <deque>
#include <flat_map>
#include <functional>
#include <string>
#include <vector>

#include <iterator>

Expand Down
1 change: 1 addition & 0 deletions libcxx/test/std/containers/container.adaptors/flat.map/incomplete_type.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// type.

#include <flat_map>
#include <vector>

struct A {
using Map = std::flat_map<A, A>;
Expand Down
Loading