Page MenuHomePhabricator

[libc++] Split out `<__functional_{ops,search}>` from `<functional>`. NFCI
AbandonedPublic

Authored by Quuxplusone on Mar 21 2021, 2:35 PM.

Details

Reviewers
ldionne
cjdb
Group Reviewers
Restricted Project
Summary

This comes out of my comments on D99041. The general strategy is:

  • Split out __search into <__functional_search>, so <algorithm> can include <__functional_search> instead of <functional>
  • Split out the lifted operators into <__functional_ops>, so many headers (notably <valarray>, <numeric>, and <set>) can include <__functional_ops> instead of <functional>

Diff Detail

Event Timeline

Quuxplusone created this revision.Mar 21 2021, 2:35 PM
Quuxplusone requested review of this revision.Mar 21 2021, 2:35 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMar 21 2021, 2:35 PM
cjdb requested changes to this revision.Mar 21 2021, 3:32 PM

LGTM except for the change to <concepts>. I think we should merge both, and then take a discussion to the mailing list for broader consideration.

libcxx/include/concepts
394 ↗(On Diff #332181)

I'm against this change, per my reason in D99041.

This revision now requires changes to proceed.Mar 21 2021, 3:32 PM

This mostly LGTM. Thanks for the cleanup. It will be great to rely less on <functional>.

This change shouldn't have any observable effect so it might make sense to say "NFC" in the title.

libcxx/include/__functional_search
11

Does it make sense to combine these two headers?

libcxx/include/concepts
394 ↗(On Diff #332181)

You can keep using forward that comes from utility/type_traits.

Quuxplusone added inline comments.Mar 22 2021, 8:12 AM
libcxx/include/concepts
394 ↗(On Diff #332181)

Yeah, and I know libc++ style is generally to use forward over static_cast; I was just testing the waters here. 😛 (The advantage would be avoiding a couple of function template instantiations every time this concept is used. OTOH maybe that matters only if __invoke is also changed to avoid forward.)
I'm not sure if @cjdb's comment refers to worrying about "changes to invoke that aren't done through __invoke, and so we're blind to the changes", which by now I think Tim Song has put to rest; but anyway I think my path of least resistance here is to eliminate the diffs in <concepts> and try that part again separately, maybe after more of @cjdb's patches have landed.

Revert diffs in <concepts>

zoecarver added inline comments.Mar 22 2021, 8:31 AM
libcxx/include/concepts
394 ↗(On Diff #332181)

Maybe this isn't the case with static_cast vs forward because they're so synonymous, but I think the added confusion to someone reading this isn't worth the absolutely minimal gain of one template instantiation (which will likely actually be cached for the use in __invoke anyway).

Quuxplusone retitled this revision from [libc++] Split out `<__functional_{ops,search}>` from `<functional>` to [libc++] Split out `<__functional_{ops,search}>` from `<functional>`. NFCI.Mar 22 2021, 8:35 AM
Quuxplusone edited the summary of this revision. (Show Details)
ldionne requested changes to this revision.Mar 22 2021, 8:40 AM

I would rather we use __functional/XYZ.h instead. Or if you convince me that __functional_XYZ is better instead, then we should make other headers consistent with that choice.

Also, just to clarify, you didn't modify any code at all, only moved it around, right? Your modifications to __invoke have been reverted from the patch. I checked, but I want to make double-sure that's your intent, since I'd push back on a change to the definition of the concept to use __invoke.

This revision now requires changes to proceed.Mar 22 2021, 8:40 AM

@ldionne wrote:

I would rather we use functional/XYZ.h instead. Or if you convince me that functional_XYZ is better instead, then we should make other headers consistent with that choice.

The other headers have been consistent with __functional_XYZ forever. Let's take this to Slack.

Also, just to clarify, you didn't modify any code at all, only moved it around, right?

That's right. (Now that the <concepts> change has been deleted, that is, that's right.)

cjdb added a comment.Mar 22 2021, 9:58 AM

@ldionne wrote:

I would rather we use functional/XYZ.h instead. Or if you convince me that functional_XYZ is better instead, then we should make other headers consistent with that choice.

The other headers have been consistent with __functional_XYZ forever. Let's take this to Slack.

There's a dedicated LLVM Discord server. Can we have this conversation there please?

Use __invoke instead of invoke, as a global preference. (__invoke comes from <type_traits>, which means we don't need to include <functional>. After this patch, git grep '::invoke(' returns no results, and I think we can easily keep it that way.)

Old dependency graph: https://i.imgur.com/o5Tu2Yc.png
New dependency graph: https://i.imgur.com/P6D8hEH.png

(Btw, I looked at what it would take to move all the invoke/__invoke stuff into its own header, and that turned out to be pretty icky, because result_of depends on __invoke_result_t, and result_of is mandated to be in <type_traits>. So in order to put invoke in the same header as __invoke, we'd need to either put invoke in <type_traits>, or split out __invoke into a header that's even lower-level than <type_traits>, e.g. "<type_traits> includes <__functional_invoke> includes <__type_traits_base>." That's too Byzantine for me.)

curdeius added inline comments.
libcxx/include/numeric
150

Why not this?

Unfortunately we can't use __invoke in bind_front and not_fn
without major surgery, because that causes some kind of problem
with their noexcept clauses. I think using is_nothrow_invocable<...>
is somehow strictly SFINAE-safer than using noexcept(invoke(...)).

But we can't use is_nothrow_invocable for not_fn, because what we care about there is that neither invoke nor operator! throw exceptions.

And we can't use is_nothrow_invocable for bind_front without further surgery, because apparently is_nothrow_invocable<Args...> is ill-formed! Clang wants us to pass parameter-pack expansions only to template parameters that themselves are packs, and is_nothrow_invocable<Fn, Args...> wants a non-pack as its first template parameter. Either this is really awful of C++, or I've misdiagnosed the problem.

Quuxplusone marked an inline comment as done.Apr 1 2021, 8:56 AM
Quuxplusone added inline comments.
libcxx/include/numeric
150

I don't know! Updated the diff to find out. ;)

ldionne requested changes to this revision.Apr 8 2021, 1:25 PM

FWIW, I love this patch in spirit, however it'll have to use __functional/search.h and __functional/ops.h to make any progress. I think it's worth doing.

This revision now requires changes to proceed.Apr 8 2021, 1:25 PM
Quuxplusone abandoned this revision.Jun 18 2021, 12:57 PM
Quuxplusone marked an inline comment as done.