This patch adds return type to std::erase and std::erase_if functions.
Also:
- Update __cpp_lib_erase_if to 202002L.
- Fix synopsis in unordered_map.
- Fix generate_feature_test_macro_components.py script.
Paths
| Differential D75905
[libc++][P1115][C++20] Improving the Return Value of Erase-Like Algorithms II: Free erase/erase if. ClosedPublic Authored by curdeius on Mar 10 2020, 4:20 AM.
Details
Summary This patch adds return type to std::erase and std::erase_if functions. Also:
Diff Detail
Event Timelinecurdeius added inline comments.
Comment Actions The pre-merge build results show this build as failed because of failing clang-format linter tests (https://reviews.llvm.org/harbormaster/build/52549/, https://results.llvm-merge-guard.org/amd64_debian_testing_clang8-4604/summary.html). Comment Actions I think there is a bug in the paper, but I think the return type should be difference_type not size_type. auto r = distance(a, b); return r; And std::distance returns difference_type. What do you think?
This revision now requires changes to proceed.Mar 11 2020, 8:23 PM Comment Actions It would be nice if we could remove the calls to std::difference and replace them with a count variable so that we only have to iterate once. It may require adding a __libcpp_ erase function, though. Comment Actions
Where do you see that the iteration takes place twice? std::distance on LegacyRandomAccessIterators is an O(1) operation. And all of basic_string, deque and vector iterators belong to this category.
I'm not sure if it's a bug. I think that a coherence with other APIs was intended. Apart the additions in P0646, there is already pre-C++11 size_type map::erase(key_type) as well as size_type set::erase(key_type). Otherwise, to be coherent with __libcpp_erase_if_container and avoid casting difference_type to size_type, I'd suggest returning old_size - size() instead of using distance.
Comment Actions Sorry, this slipped through my inbox somehow.
Here's the patch I made to implement the first paper [1]. @mclow.lists gave me some great review comments, which apply to this patch as well. Essentially, we never want to traverse the whole range twice (i.e. remove_if and distance). It looks like that's not a problem anymore, though. That being said, you make a good point. In all those cases, x.distance isn't a traversal of the container. Make sure that you're using the method, not the std::distance function, though.
Comment Actions Hopefully I answered your questions, @zoecarver.
Comment Actions Thanks for your patience. Regarding the code duplication, I think the only way to go is to split up libc++ into tinier headers. Trying to find a "clever" place to put common code is just a waste of time. Closed by commit rG3e895085de0a: [libc++][P1115][C++20] Improving the Return Value of Erase-Like Algorithms II… (authored by curdeius). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions Does anyone of you know why libcxx ARM buildbots fail after changing include headers? All of them fail (http://lab.llvm.org:8011/console) with something similar to (http://lab.llvm.org:8011/builders/libcxx-libcxxabi-libunwind-armv7-linux/builds/1314/steps/test.libcxx/logs/FAIL%3A libc%2B%2B%3A%3Astds_include.sh.cpp): /home/tcwg-buildslave/worker/libcxx-libcxxabi-libunwind-armv7-linux/llvm/libcxx/test/libcxx/modules/stds_include.sh.cpp:37:2: fatal error: file '/home/tcwg-buildslave/worker/libcxx-libcxxabi-libunwind-armv7-linux/llvm/libcxx/include/version' has been modified since the module file '/tmp/org.llvm.clang.11827/ModuleCache/8KDYU3/std-1WP0A9P.pcm' was built #include <vector> ^ /home/tcwg-buildslave/worker/libcxx-libcxxabi-libunwind-armv7-linux/llvm/libcxx/test/libcxx/modules/stds_include.sh.cpp:37:2: note: please rebuild precompiled header '/tmp/org.llvm.clang.11827/ModuleCache/8KDYU3/std-1WP0A9P.pcm' 1 error generated. It happens whenever an include is modified and it seems that the precompiled headers are not updated accordingly. Do you know if there's a fix? Do you know who's the maintainer of these bots? Comment Actions
I actually think it's a module cache invalidation bug. I think @EricWF has more context?
Revision Contents
Diff 261635 libcxx/docs/FeatureTestMacroTable.rst
libcxx/include/deque
libcxx/include/forward_list
libcxx/include/functional
libcxx/include/list
libcxx/include/map
libcxx/include/set
libcxx/include/string
libcxx/include/unordered_map
libcxx/include/unordered_set
libcxx/include/vector
libcxx/include/version
libcxx/test/std/containers/associative/map/map.erasure/erase_if.pass.cpp
libcxx/test/std/containers/associative/multimap/multimap.erasure/erase_if.pass.cpp
libcxx/test/std/containers/associative/multiset/multiset.erasure/erase_if.pass.cpp
libcxx/test/std/containers/associative/set/set.erasure/erase_if.pass.cpp
libcxx/test/std/containers/sequences/deque/deque.erasure/erase.pass.cpp
libcxx/test/std/containers/sequences/deque/deque.erasure/erase_if.pass.cpp
libcxx/test/std/containers/sequences/forwardlist/forwardlist.erasure/erase.pass.cpp
libcxx/test/std/containers/sequences/forwardlist/forwardlist.erasure/erase_if.pass.cpp
libcxx/test/std/containers/sequences/list/list.erasure/erase.pass.cpp
libcxx/test/std/containers/sequences/list/list.erasure/erase_if.pass.cpp
libcxx/test/std/containers/sequences/vector/vector.erasure/erase.pass.cpp
libcxx/test/std/containers/sequences/vector/vector.erasure/erase_if.pass.cpp
libcxx/test/std/containers/unord/unord.map/erase_if.pass.cpp
libcxx/test/std/containers/unord/unord.multimap/erase_if.pass.cpp
libcxx/test/std/containers/unord/unord.multiset/erase_if.pass.cpp
libcxx/test/std/containers/unord/unord.set/erase_if.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/deque.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/forward_list.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/list.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/map.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/set.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/string.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/unordered_map.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/unordered_set.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/vector.version.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp
libcxx/test/std/strings/strings.erasure/erase.pass.cpp
libcxx/test/std/strings/strings.erasure/erase_if.pass.cpp
libcxx/utils/generate_feature_test_macro_components.py
libcxx/www/cxx2a_status.html
|
For deque, string and vector, it might be nice to refactor this repetitive code (e.g. to a __libcpp_erase_container function, analogue to __libcpp_erase_if_container).
However, I don't know in which header it should be put as these 3 headers have no good candidate being a common include.
Note that __libcpp_erase_if_container is in <functional> and that doesn't pose any problems for maps/sets as they already include it.
Vector and string already include <__functional_base> but deque doesn't, so I'm not sure what should be the best solution. Creating a new helper header? Leaving it unrefactored?
Possible implementation of the helper: