Skip to content

Commit 64673ed

Browse files
huixie90huixie90
committed
review comments
1 parent 7e95a82 commit 64673ed

20 files changed

+264
-202
lines changed

‎libcxx/include/__flat_map/flat_map.h

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -247,26 +247,29 @@ class flat_map {
247247
is_nothrow_default_constructible_v<_Compare>)
248248
: __containers_(), __compare_() {}
249249

250+
_LIBCPP_HIDE_FROM_ABI flat_map(const flat_map&) = default;
251+
250252
// The copy/move constructors are not specified in the spec, which means they should be defaulted.
251253
// However, the move constructor can potentially leave a moved-from object in an inconsistent
252254
// state if an exception is thrown.
253-
_LIBCPP_HIDE_FROM_ABI flat_map(const flat_map&) = default;
254-
255-
// gcc does not like the `throw` keyword in a conditional noexcept function
256-
// split the move constructor into two
257-
_LIBCPP_HIDE_FROM_ABI flat_map(flat_map&& __other) try
255+
_LIBCPP_HIDE_FROM_ABI flat_map(flat_map&& __other) noexcept(
256+
is_nothrow_move_constructible_v<_KeyContainer> && is_nothrow_move_constructible_v<_MappedContainer> &&
257+
is_nothrow_move_constructible_v<_Compare>)
258+
# if _LIBCPP_HAS_EXCEPTIONS
259+
try
260+
# endif // _LIBCPP_HAS_EXCEPTIONS
258261
: __containers_(std::move(__other.__containers_)), __compare_(std::move(__other.__compare_)) {
259262
__other.clear();
263+
# if _LIBCPP_HAS_EXCEPTIONS
260264
} catch (...) {
261265
__other.clear();
262-
throw;
263-
}
264-
265-
_LIBCPP_HIDE_FROM_ABI flat_map(flat_map&& __other) noexcept
266-
requires is_nothrow_move_constructible_v<_KeyContainer> && is_nothrow_move_constructible_v<_MappedContainer> &&
267-
is_nothrow_move_constructible_v<_Compare>
268-
: __containers_(std::move(__other.__containers_)), __compare_(std::move(__other.__compare_)) {
269-
__other.clear();
266+
if constexpr (is_nothrow_move_constructible_v<_KeyContainer> && is_nothrow_move_constructible_v<_MappedContainer> &&
267+
is_nothrow_move_constructible_v<_Compare>) {
268+
// gcc does not like the `throw` keyword in a conditional noexcept function
269+
// split the move constructor into two
270+
throw;
271+
}
272+
# endif // _LIBCPP_HAS_EXCEPTIONS
270273
}
271274

272275
template <class _Allocator>
@@ -280,16 +283,21 @@ class flat_map {
280283

281284
template <class _Allocator>
282285
requires __allocator_ctor_constraint<_Allocator>
283-
_LIBCPP_HIDE_FROM_ABI flat_map(flat_map&& __other, const _Allocator& __alloc) try
286+
_LIBCPP_HIDE_FROM_ABI flat_map(flat_map&& __other, const _Allocator& __alloc)
287+
# if _LIBCPP_HAS_EXCEPTIONS
288+
try
289+
# endif // _LIBCPP_HAS_EXCEPTIONS
284290
: flat_map(__ctor_uses_allocator_tag{},
285291
__alloc,
286292
std::move(__other.__containers_.keys),
287293
std::move(__other.__containers_.values),
288294
std::move(__other.__compare_)) {
289295
__other.clear();
296+
# if _LIBCPP_HAS_EXCEPTIONS
290297
} catch (...) {
291298
__other.clear();
292299
throw;
300+
# endif // _LIBCPP_HAS_EXCEPTIONS
293301
}
294302

295303
_LIBCPP_HIDE_FROM_ABI flat_map(
@@ -575,7 +583,6 @@ class flat_map {
575583
template <class _Kp>
576584
requires __is_compare_transparent
577585
_LIBCPP_HIDE_FROM_ABI mapped_type& at(const _Kp& __x) {
578-
static_assert(requires { find(__x); }, "flat_map::at(const K& x): find(x) needs to be well-formed");
579586
auto __it = find(__x);
580587
if (__it == end()) {
581588
std::__throw_out_of_range("flat_map::at(const K&): Key does not exist");
@@ -586,7 +593,6 @@ class flat_map {
586593
template <class _Kp>
587594
requires __is_compare_transparent
588595
_LIBCPP_HIDE_FROM_ABI const mapped_type& at(const _Kp& __x) const {
589-
static_assert(requires { find(__x); }, "flat_map::at(const K& x) const: find(x) needs to be well-formed");
590596
auto __it = find(__x);
591597
if (__it == end()) {
592598
std::__throw_out_of_range("flat_map::at(const K&) const: Key does not exist");
@@ -1124,8 +1130,11 @@ class flat_map {
11241130
} else {
11251131
// In this case, we know the values are just like before we attempted emplacement,
11261132
// and we also know that the keys have been emplaced successfully. Just roll back the keys.
1133+
# if _LIBCPP_HAS_EXCEPTIONS
11271134
try {
1135+
# endif // _LIBCPP_HAS_EXCEPTIONS
11281136
__containers_.keys.erase(__key_it);
1137+
# if _LIBCPP_HAS_EXCEPTIONS
11291138
} catch (...) {
11301139
// Now things are funky for real. We're failing to rollback the keys.
11311140
// Just give up and clear the whole thing.
@@ -1134,6 +1143,7 @@ class flat_map {
11341143
// original value-emplacement exception propagate normally.
11351144
clear() /* noexcept */;
11361145
}
1146+
# endif // _LIBCPP_HAS_EXCEPTIONS
11371147
}
11381148
});
11391149
auto __mapped_it = __containers_.values.emplace(__it_mapped, std::forward<_MArgs>(__mapped_args)...);

‎libcxx/test/std/containers/container.adaptors/flat.map/flat.map.cons/move.pass.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ int main(int, char**) {
7979
M m1 = M({1, 2, 3}, {1, 2, 3});
8080
M m2 = std::move(m1);
8181
assert(m2.size() == 3);
82-
assert(m1.keys().size() == m1.values().size());
82+
check_invariant(m1);
8383
LIBCPP_ASSERT(m1.empty());
8484
LIBCPP_ASSERT(m1.keys().size() == 0);
8585
LIBCPP_ASSERT(m1.values().size() == 0);

‎libcxx/test/std/containers/container.adaptors/flat.map/flat.map.cons/move_alloc.pass.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ int main(int, char**) {
103103
M m1 = M({1, 2, 3}, {1, 2, 3});
104104
M m2(std::move(m1), std::allocator<int>{});
105105
assert(m2.size() == 3);
106-
assert(m1.keys().size() == m1.values().size());
106+
check_invariant(m1);
107107
LIBCPP_ASSERT(m1.empty());
108108
LIBCPP_ASSERT(m1.keys().size() == 0);
109109
LIBCPP_ASSERT(m1.values().size() == 0);

‎libcxx/test/std/containers/container.adaptors/flat.map/flat.map.cons/move_assign.pass.cpp

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -89,22 +89,6 @@ int main(int, char**) {
8989
mo.insert({"foo", 1});
9090
assert(mo.begin()->first.get_allocator().resource() == &mr1);
9191
}
92-
{
93-
// A moved-from flat_map maintains its class invariant in the presence of moved-from comparators.
94-
using C = std::function<bool(int, int)>;
95-
using M = std::flat_map<int, int, C>;
96-
M mo = M({{1, 3}, {2, 2}, {3, 1}}, std::less<int>());
97-
M m = M({{1, 1}, {2, 2}}, std::greater<int>());
98-
m = std::move(mo);
99-
assert(m.size() == 3);
100-
assert(std::is_sorted(m.begin(), m.end(), m.value_comp()));
101-
assert(m.key_comp()(1, 2) == true);
10292

103-
assert(std::is_sorted(mo.begin(), mo.end(), mo.value_comp()));
104-
LIBCPP_ASSERT(m.key_comp()(1, 2) == true);
105-
LIBCPP_ASSERT(mo.empty());
106-
mo.insert({{1, 3}, {2, 2}, {3, 1}}); // insert has no preconditions
107-
LIBCPP_ASSERT(m == mo);
108-
}
10993
return 0;
11094
}

‎libcxx/test/std/containers/container.adaptors/flat.map/flat.map.cons/move_assign_clears.pass.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ int main(int, char**) {
9595
M m2 = M({1, 2}, {1, 2});
9696
m2 = std::move(m1);
9797
assert(m2.size() == 3);
98-
assert(m1.keys().size() == m1.values().size());
98+
check_invariant(m1);
9999
LIBCPP_ASSERT(m1.empty());
100100
LIBCPP_ASSERT(m1.keys().size() == 0);
101101
LIBCPP_ASSERT(m1.values().size() == 0);

‎libcxx/test/std/containers/container.adaptors/flat.map/flat.map.cons/move_assign_noexcept.pass.cpp

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,14 @@ struct MoveSensitiveComp {
4141
bool is_moved_from_ = false;
4242
};
4343

44+
struct MoveThrowsComp {
45+
MoveThrowsComp(MoveThrowsComp&&) noexcept(false);
46+
MoveThrowsComp(const MoveThrowsComp&) noexcept(true);
47+
MoveThrowsComp& operator=(MoveThrowsComp&&) noexcept(false);
48+
MoveThrowsComp& operator=(const MoveThrowsComp&) noexcept(true);
49+
bool operator()(const auto&, const auto&) const;
50+
};
51+
4452
int main(int, char**) {
4553
{
4654
using C = std::flat_map<int, int>;
@@ -82,14 +90,11 @@ int main(int, char**) {
8290
std::vector<MoveOnly, other_allocator<MoveOnly>>>;
8391
LIBCPP_STATIC_ASSERT(std::is_nothrow_move_assignable_v<C>);
8492
}
85-
/*
86-
why? std::function move assignment is noexcept
8793
{
88-
// Test with a comparator that throws on copy-assignment.
89-
using C = std::flat_map<int, int, std::function<bool(int, int)>>;
94+
// Test with a comparator that throws on move-assignment.
95+
using C = std::flat_map<int, int, MoveThrowsComp>;
9096
LIBCPP_STATIC_ASSERT(!std::is_nothrow_move_assignable_v<C>);
9197
}
92-
*/
9398
{
9499
// Test with a container that throws on move-assignment.
95100
using C = std::flat_map<int, int, std::less<int>, std::pmr::vector<int>, std::vector<int>>;
@@ -100,18 +105,6 @@ int main(int, char**) {
100105
using C = std::flat_map<int, int, std::less<int>, std::vector<int>, std::pmr::vector<int>>;
101106
static_assert(!std::is_nothrow_move_assignable_v<C>);
102107
}
103-
/* why?
104-
{
105-
// Moving the flat_map copies the comparator (to support std::function comparators)
106-
using C = std::flat_map<int, int, MoveSensitiveComp>;
107-
LIBCPP_STATIC_ASSERT(std::is_nothrow_move_assignable_v<C>);
108-
C c;
109-
assert(!c.key_comp().is_moved_from_);
110-
C d;
111-
d = std::move(c);
112-
LIBCPP_ASSERT(!c.key_comp().is_moved_from_);
113-
assert(!d.key_comp().is_moved_from_);
114-
}
115-
*/
108+
116109
return 0;
117110
}

‎libcxx/test/std/containers/container.adaptors/flat.map/flat.map.cons/move_exceptions.pass.cpp

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <utility>
2222
#include <vector>
2323

24+
#include "../helpers.h"
2425
#include "test_macros.h"
2526

2627
static int countdown = 0;
@@ -49,9 +50,7 @@ int main(int, char**) {
4950
assert(x == 42);
5051
}
5152
// The source flat_map maintains its class invariant.
52-
assert(mo.keys().size() == mo.values().size());
53-
assert(std::is_sorted(mo.begin(), mo.end()));
54-
assert(std::adjacent_find(mo.keys().begin(), mo.keys().end()) == mo.keys().end());
53+
check_invariant(mo);
5554
LIBCPP_ASSERT(mo.empty());
5655
}
5756
{
@@ -65,9 +64,7 @@ int main(int, char**) {
6564
assert(x == 42);
6665
}
6766
// The source flat_map maintains its class invariant.
68-
assert(mo.keys().size() == mo.values().size());
69-
assert(std::is_sorted(mo.begin(), mo.end()));
70-
assert(std::adjacent_find(mo.keys().begin(), mo.keys().end()) == mo.keys().end());
67+
check_invariant(mo);
7168
LIBCPP_ASSERT(mo.empty());
7269
}
7370
return 0;

‎libcxx/test/std/containers/container.adaptors/flat.map/flat.map.cons/move_noexcept.pass.cpp

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,10 @@ struct ThrowingMoveAllocator {
4040
friend bool operator==(ThrowingMoveAllocator, ThrowingMoveAllocator) = default;
4141
};
4242

43-
struct ThrowingCopyComp {
44-
ThrowingCopyComp() = default;
45-
ThrowingCopyComp(const ThrowingCopyComp&) noexcept(false) {}
46-
ThrowingCopyComp(ThrowingCopyComp&&) noexcept {}
43+
struct ThrowingMoveComp {
44+
ThrowingMoveComp() = default;
45+
ThrowingMoveComp(const ThrowingMoveComp&) noexcept(true) {}
46+
ThrowingMoveComp(ThrowingMoveComp&&) noexcept(false) {}
4747
bool operator()(const auto&, const auto&) const { return false; }
4848
};
4949

@@ -92,11 +92,9 @@ int main(int, char**) {
9292
}
9393
#endif // _LIBCPP_VERSION
9494
{
95-
// Comparator fails to be nothrow-copy-constructible
96-
using C = std::flat_map<int, int, ThrowingCopyComp>;
97-
//todo: why???
98-
//static_assert(!std::is_nothrow_move_constructible_v<C>);
99-
static_assert(std::is_nothrow_move_constructible_v<C>);
95+
// Comparator fails to be nothrow-move-constructible
96+
using C = std::flat_map<int, int, ThrowingMoveComp>;
97+
static_assert(!std::is_nothrow_move_constructible_v<C>);
10098
C c;
10199
C d = std::move(c);
102100
}

‎libcxx/test/std/containers/container.adaptors/flat.map/flat.map.erasure/erase_if_exceptions.pass.cpp

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include <utility>
2626
#include <vector>
2727

28+
#include "../helpers.h"
2829
#include "test_macros.h"
2930

3031
struct Counter {
@@ -83,9 +84,7 @@ int main(int, char**) {
8384
break;
8485
} catch (int ex) {
8586
assert(ex == 42);
86-
assert(m.keys().size() == m.values().size()); // still sized correctly
87-
assert(std::is_sorted(m.begin(), m.end())); // still sorted
88-
assert(std::adjacent_find(m.begin(), m.end()) == m.end()); // still contains no duplicates
87+
check_invariant(m);
8988
LIBCPP_ASSERT(m.empty() || std::equal(m.begin(), m.end(), expected, expected + 8));
9089
if (g_counter.throws == 1) {
9190
// We reached the first throw but not the second throw.
@@ -112,9 +111,7 @@ int main(int, char**) {
112111
break;
113112
} catch (int ex) {
114113
assert(ex == 42);
115-
assert(m.keys().size() == m.values().size()); // still sized correctly
116-
assert(std::is_sorted(m.begin(), m.end())); // still sorted
117-
assert(std::adjacent_find(m.begin(), m.end()) == m.end()); // still contains no duplicates
114+
check_invariant(m);
118115
LIBCPP_ASSERT(m.empty() || std::equal(m.begin(), m.end(), expected, expected + 8));
119116
if (g_counter.throws == 1) {
120117
// We reached the first throw but not the second throw.
@@ -144,9 +141,7 @@ int main(int, char**) {
144141
break;
145142
} catch (int ex) {
146143
assert(ex == 42);
147-
assert(m.keys().size() == m.values().size()); // still sized correctly
148-
assert(std::is_sorted(m.begin(), m.end())); // still sorted
149-
assert(std::adjacent_find(m.begin(), m.end()) == m.end()); // still contains no duplicates
144+
check_invariant(m);
150145
LIBCPP_ASSERT(m.empty() || std::equal(m.begin(), m.end(), expected, expected + 8));
151146
if (g_counter.throws == 1) {
152147
// We reached the first throw but not the second throw.

0 commit comments

Comments
 (0)