[libc++] Fix the rest of __gnu_cxx::hash_XXX copy construction #160525
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: None (asmok-g) ChangesFull diff: https://github.com/llvm/llvm-project/pull/160525.diff 5 Files Affected:
diff --git a/libcxx/include/ext/hash_map b/libcxx/include/ext/hash_map
index 01ca7498f0cc1..09c981131ff96 100644
--- a/libcxx/include/ext/hash_map
+++ b/libcxx/include/ext/hash_map
@@ -570,10 +570,7 @@ hash_map<_Key, _Tp, _Hash, _Pred, _Alloc>::hash_map(
}
template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
-hash_map<_Key, _Tp, _Hash, _Pred, _Alloc>::hash_map(const hash_map& __u) : __table_(__u.__table_) {
- __table_.__rehash_unique(__u.bucket_count());
- insert(__u.begin(), __u.end());
-}
+hash_map<_Key, _Tp, _Hash, _Pred, _Alloc>::hash_map(const hash_map& __u) : __table_(__u.__table_) {}
template <class _Key, class _Tp, class _Hash, class _Pred, class _Alloc>
typename hash_map<_Key, _Tp, _Hash, _Pred, _Alloc>::__node_holder
diff --git a/libcxx/include/ext/hash_set b/libcxx/include/ext/hash_set
index 62a7a0dbcffb9..56aa4d8a47eeb 100644
--- a/libcxx/include/ext/hash_set
+++ b/libcxx/include/ext/hash_set
@@ -356,10 +356,7 @@ hash_set<_Value, _Hash, _Pred, _Alloc>::hash_set(
}
template <class _Value, class _Hash, class _Pred, class _Alloc>
-hash_set<_Value, _Hash, _Pred, _Alloc>::hash_set(const hash_set& __u) : __table_(__u.__table_) {
- __table_.__rehash_unique(__u.bucket_count());
- insert(__u.begin(), __u.end());
-}
+hash_set<_Value, _Hash, _Pred, _Alloc>::hash_set(const hash_set& __u) : __table_(__u.__table_) {}
template <class _Value, class _Hash, class _Pred, class _Alloc>
template <class _InputIterator>
@@ -534,10 +531,7 @@ hash_multiset<_Value, _Hash, _Pred, _Alloc>::hash_multiset(
}
template <class _Value, class _Hash, class _Pred, class _Alloc>
-hash_multiset<_Value, _Hash, _Pred, _Alloc>::hash_multiset(const hash_multiset& __u) : __table_(__u.__table_) {
- __table_.__rehash_multi(__u.bucket_count());
- insert(__u.begin(), __u.end());
-}
+hash_multiset<_Value, _Hash, _Pred, _Alloc>::hash_multiset(const hash_multiset& __u) : __table_(__u.__table_) {}
template <class _Value, class _Hash, class _Pred, class _Alloc>
template <class _InputIterator>
diff --git a/libcxx/test/extensions/gnu/hash_map/copy.pass.cpp b/libcxx/test/extensions/gnu/hash_map/copy.pass.cpp
new file mode 100644
index 0000000000000..f54e69bc20fd7
--- /dev/null
+++ b/libcxx/test/extensions/gnu/hash_map/copy.pass.cpp
@@ -0,0 +1,27 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// ADDITIONAL_COMPILE_FLAGS: -Wno-deprecated
+
+// hash_multimap::hash_multimap(const hash_multimap&)
+
+#include <cassert>
+#include <ext/hash_map>
+
+int main(int, char**) {
+ __gnu_cxx::hash_map<int, int> map;
+
+ map.insert(std::make_pair(1, 1));
+ map.insert(std::make_pair(1, 1));
+
+ auto map2 = map;
+
+ assert(map2.size() == 2);
+
+ return 0;
+}
diff --git a/libcxx/test/extensions/gnu/hash_multiset/copy.pass.cpp b/libcxx/test/extensions/gnu/hash_multiset/copy.pass.cpp
new file mode 100644
index 0000000000000..c8f57875890da
--- /dev/null
+++ b/libcxx/test/extensions/gnu/hash_multiset/copy.pass.cpp
@@ -0,0 +1,27 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// ADDITIONAL_COMPILE_FLAGS: -Wno-deprecated
+
+// hash_multimap::hash_multimap(const hash_multimap&)
+
+#include <cassert>
+#include <ext/hash_set>
+
+int main(int, char**) {
+ __gnu_cxx::hash_multiset<int> set;
+
+ set.insert(1);
+ set.insert(1);
+
+ auto set2 = set;
+
+ assert(set2.size() == 2);
+
+ return 0;
+}
diff --git a/libcxx/test/extensions/gnu/hash_set/copy.pass.cpp b/libcxx/test/extensions/gnu/hash_set/copy.pass.cpp
new file mode 100644
index 0000000000000..cf4641aa91534
--- /dev/null
+++ b/libcxx/test/extensions/gnu/hash_set/copy.pass.cpp
@@ -0,0 +1,27 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// ADDITIONAL_COMPILE_FLAGS: -Wno-deprecated
+
+// hash_multimap::hash_multimap(const hash_multimap&)
+
+#include <cassert>
+#include <ext/hash_set>
+
+int main(int, char**) {
+ __gnu_cxx::hash_set<int> set;
+
+ set.insert(1);
+ set.insert(2);
+
+ auto set2 = set;
+
+ assert(set2.size() == 2);
+
+ return 0;
+}
|
|
@ldionne @philnik777 I hope you won't mind, if we land this. We still have some code that depends on these containers, and this remains one of the last blockers holding our next internal release for quite a while already. |
@philnik777 oh nice, thanks for landing the second fix as well! It would be nice, if you let us know about it by responding to the comments on #151951, which caused the problem and where the issue was reported, or by answering a comment on #160043, which highlighted the incompleteness of the fix, or at the very least by including the context into the descriptions of your fixes (#160043 and #160466, neither of which mentions, which commit or which PR caused the problem this commit is fixing). Having cross-references in PRs and commit messages really helps understanding what happened and whether it was addressed, while silently landing fixes without responding to the reports results in confusion and sometimes (like in this case) in extra work being done by others. As for the non-multi containers, my understanding is that they are also affected, but in a bit different way: their constructor will just repeat all the insertions of the elements from the source container, effectively being ~2x slower. I don't have any tests or benchmarks to back up my words, but just from looking at the code it looks like it is so. |
Yeah, sorry. My communication was atrocious here.
I don't know whether I'd call that "affected". The |
I totally agree with the sentiment, and I was frankly surprised to see the _gnu_cxx::hash... containers being still used. I'm now trying to get rid of the usages completely, but there's some third-party code, which requires more careful approach, and some cases that are less trivial to migrate due to custom hashers etc. So completely breaking those would be undesired for us. In this particular case the fix is trivial, and it aligns well with the related changes in hash_multi.* containers. It seems to be a net positive, so I don't see a reason to hold this PR. Do you have any particular concerns with landing the rest of the fix in this PR? |
No description provided.