Page MenuHomePhabricator

[libc++] Remove default_searcher from experimental/functional
AbandonedPublic

Authored by jloser on Feb 7 2022, 3:38 PM.

Details

Reviewers
ldionne
Quuxplusone
Mordante
var-const
philnik
Group Reviewers
Restricted Project
Summary

std::default_searcher has been implemented for a while now
(introduced in d835e59211883) - far more than two llvm releases. So, remove it
and std::experimental::make_default_searcher from <experimental/functional>.

There already exists tests for std::default_searcher in
libcxx/test/std/utilities/function.objects/func.search/func.search.default, so
there is no loss in test coverage.

Diff Detail

Event Timeline

jloser requested review of this revision.Feb 7 2022, 3:38 PM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2022, 3:38 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jloser updated this revision to Diff 406653.Feb 7 2022, 4:52 PM

Update synopsis in experimental/functional

Would you be interested in looking into the other stuff defined in the header? I'm guessing that these things can also be removed.

jloser added a comment.Feb 7 2022, 5:22 PM

Would you be interested in looking into the other stuff defined in the header? I'm guessing that these things can also be removed.

Yep, I have a follow-up planned for booyer moore searcher. We already have those tests XFAILed for libc++ for std::booyer_moore_searcher. :)

ldionne requested changes to this revision.Feb 8 2022, 10:10 AM

LGTM, but please add a release note mentioning the removal.

libcxx/include/experimental/functional
106–107

Please don't bump the standard version in this patch, since it's technically an unrelated change (IIUC).

This revision now requires changes to proceed.Feb 8 2022, 10:10 AM
jloser updated this revision to Diff 406893.Feb 8 2022, 10:49 AM

Add release note

libcxx/include/experimental/functional
106–107

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.

Quuxplusone requested changes to this revision.Feb 8 2022, 11:06 AM
Quuxplusone added inline comments.
libcxx/include/experimental/functional
106–107

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.

107

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.

This revision now requires changes to proceed.Feb 8 2022, 11:06 AM
jloser updated this revision to Diff 406933.Feb 8 2022, 12:30 PM
jloser edited the summary of this revision. (Show Details)

Remove std::experimental::make_default_searcher and update release note to reflect this

jloser added inline comments.Feb 8 2022, 1:22 PM
libcxx/include/experimental/functional
106–107

Valid argument. I've removed std::experimental::make_default_searcher.

107

Agree re the ordering. We could take std::experimental::boyer_moore_{,horspool_}searcher and figure out the loose ends to put them into std and then remove all the searchers from experimental/functional at once. If we do that, then I'll close this PR, and I might volunteer to implement the searchers into std to complete the work effectively. The alternative is to piecemeal the removals by doing just default_searcher and make_default_searcher since std::default_searcher already is implemented today, which is what this PR is.

@ldionne thoughts on piecemeal removal vs all-at-once? I don't think I'd get to the other searchers for a little while if we want to do the all-at-once approach.

jloser marked 2 inline comments as done.Feb 8 2022, 1:22 PM
ldionne added inline comments.Feb 8 2022, 2:00 PM
libcxx/include/experimental/functional
107

I have to agree with Arthur here -- if a user sees that we removed std::experimental::default_searcher, the next thing they will try to do is migrate any uses of std::experimental::boyer_moore_searcher to std::boyer_moore_searcher, only to see that fail. If we don't remove all at once, we're creating churn for the users, and I think we'd like to avoid that. Sorry I didn't catch it before, but I think the right call would be not to remove those at all until we can remove all of them in favour of the std:: version.

jloser abandoned this revision.Aug 20 2022, 4:23 PM

This work is already done - abandoning this patch.

Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 4:23 PM