[libcxx-commits] [PATCH] D119198: [libc++] Remove default_searcher from experimental/functional
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Feb 8 11:06:27 PST 2022
Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/include/experimental/functional:116
+#if _LIBCPP_STD_VER > 11
template<class _Key, class _Value, class _Hash, class _BinaryPredicate, bool /*useArray*/> class _BMSkipTable;
----------------
Orthogonally: What's the deal with `std::experimental::boyer_moore_searcher`? Have we implemented `std::boyer_moore_searcher` https://en.cppreference.com/w/cpp/utility/functional/boyer_moore_searcher yet? (No. We haven't.)
Ordering-wise, it would sure have made a lot more sense for us to have implemented `std::boyer_moore_{,horspool_}searcher` all at once, and then we could yank `std::experimental::*_searcher` all at once. The current state — where you //need// `std::experimental::` for two of the searchers, but (@jloser proposes) you'll //need// `std::` for the other one — seems really unfortunate to me. But the time for us to fix that was 2 releases ago; it's not @jloser's fault that we didn't do it, and so //maybe// it shouldn't stop us from removing deprecated features according to schedule.
================
Comment at: libcxx/include/experimental/functional:106
-#if _LIBCPP_STD_VER > 11
-// default searcher
-template<class _ForwardIterator, class _BinaryPredicate = equal_to<>>
-class _LIBCPP_TEMPLATE_VIS default_searcher {
-public:
- _LIBCPP_INLINE_VISIBILITY
- default_searcher(_ForwardIterator __f, _ForwardIterator __l,
- _BinaryPredicate __p = _BinaryPredicate())
- : __first_(__f), __last_(__l), __pred_(__p) {}
-
- template <typename _ForwardIterator2>
- _LIBCPP_INLINE_VISIBILITY
- pair<_ForwardIterator2, _ForwardIterator2>
- operator () (_ForwardIterator2 __f, _ForwardIterator2 __l) const
- {
- return _VSTD::__search(__f, __l, __first_, __last_, __pred_,
- typename iterator_traits<_ForwardIterator>::iterator_category(),
- typename iterator_traits<_ForwardIterator2>::iterator_category());
- }
-
-private:
- _ForwardIterator __first_;
- _ForwardIterator __last_;
- _BinaryPredicate __pred_;
- };
-
+#if _LIBCPP_STD_VER > 14
template<class _ForwardIterator, class _BinaryPredicate = equal_to<>>
----------------
jloser wrote:
> ldionne wrote:
> > Please don't bump the standard version in this patch, since it's technically an unrelated change (IIUC).
> It's required if we want `make_default_searcher` to work at all since `std::default_searcher` is C++17-or-later only (https://github.com/llvm/llvm-project/blob/main/libcxx/include/__functional/default_searcher.h#L25). Otherwise, we can't remove `std::experimental::default_searcher` if we want these to work in C++14-or-later and not C++17-or-later.
>
> We could remove `make_default_searcher` entirely, but I think that'd be for a separate patch and I don't see the reason to do so right now.
If you're removing `std::experimental::default_searcher`, you should definitely remove `std::experimental::make_default_searcher` at the same time. The two are tied inextricably together — what you've got now is analogous to removing `std::tr1::shared_ptr` but keeping `std::tr1::weak_ptr`!
Think about the user experience here: as a user of `std::experimental::{make_,}default_searcher`, would you rather have the whole thing yanked, or would you rather have `std::experimental::make_default_searcher` yanked //only// in C++14 mode, and in C++17 mode produce a different return type than it used to? Yanking the whole thing will be much saner from the user's POV.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119198/new/
https://reviews.llvm.org/D119198
More information about the libcxx-commits
mailing list