[libcxx] moves `std::invoke` into `__functional_base`

[cjdb]

Summary

Including <concepts> in other standard library headers (such as
<iterator>) creates circular dependencies due to <functional>.
Since <concepts> only needs std::invoke from <functional>, the
easiest, fastest, and cleanest way to eliminate the circular dep is to
move std::invoke into __functional_base.

This has the added advantage of <concepts> not transitively importing
<functional>.

[• Quuxplusone]

• It's confusing to me that the header __invoke doesn't actually define __invoke, but rather invoke.
• Most libc++-internal consumers actually care about __invoke, not invoke. (__invoke is so fundamental that I would have expected it in type_traits with the rest of the fundamentals, but it looks like it's in __functional_base instead.)
• I suggest that you just move invoke into the existing __functional_base header, alongside ref et al. That'll be a much smaller patch, and also unifies invoke and __invoke in the same place, which is helpful to maintainers.
• I'd like your opinion (and maybe @tcanens too?) on whether libc++ could just use _VSTD::__invoke inside all the invocable concepts, instead of _VSTD::invoke. Using __invoke wouldn't help much AFAICT. It would very slightly, negligibly lessen the compile-time burden, and it would reduce the coupling between these concepts and the rest of the C++17 library. (This would merely help libc++ support a "C++14 plus concepts" mode, which is a crazy and pointless idea.) But I'm still wondering whether it would hurt.

• We should try to get to the point where no C++ standard header includes <functional>.

I might even get nerdsniped into making that patch...

[cjdb]

applies @Quuxplusone's point about __functional_base

[cjdb]

In D99041#2640282, @Quuxplusone wrote:

• It's confusing to me that the header __invoke doesn't actually define __invoke, but rather invoke.

🤷 that's already __functional_base. I named it __invoke instead of invoke so users don't accidentally include it. I could move it to __functional/invoke, which is 1000x clearer IMNSHO (see below).

• Most libc++-internal consumers actually care about __invoke, not invoke. (__invoke is so fundamental that I would have expected it in type_traits with the rest of the fundamentals, but it looks like it's in __functional_base instead.)

Yeah, that's a good musing. See below.

• I suggest that you just move invoke into the existing __functional_base header, alongside ref et al. That'll be a much smaller patch, and also unifies invoke and __invoke in the same place, which is helpful to maintainers.

Good observation! I was under the impression that <type_traits> depended on __functional_base and not the other way around, so I've applied this in spite of my opinion below.

• I'd like your opinion (and maybe @tcanens too?) on whether libc++ could just use _VSTD::__invoke inside all the invocable concepts, instead of _VSTD::invoke. Using __invoke wouldn't help much AFAICT. It would very slightly, negligibly lessen the compile-time burden, and it would reduce the coupling between these concepts and the rest of the C++17 library. (This would merely help libc++ support a "C++14 plus concepts" mode, which is a crazy and pointless idea.) But I'm still wondering whether it would hurt.

Considered this, but chickened out because a change to invoke might not reflect updates to invocable without manual intervention. I'd prefer to keep concepts verbatim, even though I strongly suspect any changes to invoke will likely go through INVOKE Also note that @ldionne has strong opinions on keeping things as close to wording as possible.

• We should try to get to the point where no C++ standard header includes <functional>.

(See here.)

Agreed, +100, etc.. I'm considering writing to libc++ mailing to suggest we move all names into their own headers (e.g. <__functional/invoke>, <__functional/function>, etc.) and the standard headers just include <__header_name/*> (where * is a placeholder and not what we really include). I don't have the time to do this myself, but will review any and all patches that incrementally move us in that direction. It also gives us a rare opportunity to review the libc++ clang-format style, if the group wants change (I'm ambivalent, but it's a point worth noting before we start the endeavour).

I might even get nerdsniped into making that patch...

No objections from me. See above ;-)

[• Quuxplusone]

FWIW, I am [now] willing to be one of the +1s on this patch [moving invoke into <__functional_base>]; but I also think it doesn't go far enough, and have indeed been nerdsniped into making my own version. 😛 My version so far is trying to

• change <concepts> to use __invoke instead of invoke, so it can include <__functional_base> instead of <functional> (although this patch's solution also seems fine)
• 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>
• move __libcpp_erase_if_container somewhere else (maybe <iterator>) and ADL-proof its callers (this will likely be severable)

[zoecarver]

In D99041#2640306, @cjdb wrote:

In D99041#2640282, @Quuxplusone wrote:

• I'd like your opinion (and maybe @tcanens too?) on whether libc++ could just use _VSTD::__invoke inside all the invocable concepts, instead of _VSTD::invoke. Using __invoke wouldn't help much AFAICT. It would very slightly, negligibly lessen the compile-time burden, and it would reduce the coupling between these concepts and the rest of the C++17 library. (This would merely help libc++ support a "C++14 plus concepts" mode, which is a crazy and pointless idea.) But I'm still wondering whether it would hurt.

Considered this, but chickened out because a change to invoke might not reflect updates to invocable without manual intervention. I'd prefer to keep concepts verbatim, even though I strongly suspect any changes to invoke will likely go through INVOKE Also note that @ldionne has strong opinions on keeping things as close to wording as possible.

If there are updates to invocable, then I assume we'd add new tests to reflect the change. Either the tests would pass, in which case we could keep using __invoke or they wouldn't and we'd update it then. Also, std::invoke is a one-liner that just calls __invoke, so I think we're OK.

That being said, I don't really see what we're getting from this change. Is it just that <concepts> would stop relying on <functional> (which very well might be worth it)?

[cjdb]

In D99041#2640407, @zoecarver wrote:

In D99041#2640306, @cjdb wrote:

In D99041#2640282, @Quuxplusone wrote:

• I'd like your opinion (and maybe @tcanens too?) on whether libc++ could just use _VSTD::__invoke inside all the invocable concepts, instead of _VSTD::invoke. Using __invoke wouldn't help much AFAICT. It would very slightly, negligibly lessen the compile-time burden, and it would reduce the coupling between these concepts and the rest of the C++17 library. (This would merely help libc++ support a "C++14 plus concepts" mode, which is a crazy and pointless idea.) But I'm still wondering whether it would hurt.

Considered this, but chickened out because a change to invoke might not reflect updates to invocable without manual intervention. I'd prefer to keep concepts verbatim, even though I strongly suspect any changes to invoke will likely go through INVOKE Also note that @ldionne has strong opinions on keeping things as close to wording as possible.

If there are updates to invocable, then I assume we'd add new tests to reflect the change. Either the tests would pass, in which case we could keep using __invoke or they wouldn't and we'd update it then. Also, std::invoke is a one-liner that just calls __invoke, so I think we're OK.

My concern isn't changes to invocable; it's changes to invoke that aren't done through __invoke, and so we're blind to the changes. @CaseyCarter, @tcanens any input here on my paranoia?

That being said, I don't really see what we're getting from this change. Is it just that <concepts> would stop relying on <functional> (which very well might be worth it)?

Yeah, that's the salient point here.

[tcanens]

The exact expression in an atomic constraint is not observable so you can certainly substitute something else if that is preferable for whatever reason.

I suspect that making invoke diverge from INVOKE will involve the dead bodies of many LWG members, mine included. I can't predict what libc++ is going to do in the unlikely event that WG21 changes both INVOKE and invoke, though. Maybe for whatever reason it will end up needing an __invoke2.

[tschuett]

Then you might want to add tests to show that

__invoke == invoke

[• Quuxplusone]

@tschuett: FYI, there are a lot of tests for __invoke already in libcxx/utilities/function.objects/func.require/bullet_*.pass.cpp.
I guess it would be interesting to run those same tests with -DINVOKE=__invoke in all modes and additionally with -DINVOKE=invoke in C++17+ modes. (And ditto the constexpr tests in C++20+ modes.) Maybe someone who knows more than I do about llvm-lit will volunteer to make that patch. :)

[ldionne]

Regarding using __invoke in the concept: I'm against that, the reason is not about introducing a bug in the future, it's just about reasoning. I want to be able to read the concept and have it be pretty much verbatim the same as specified in the Standard. There are better ways to get rid of header dependencies than changing the code to use a proxy function instead of the real one.

I am in the camp of @cjdb here:

Agreed, +100, etc.. I'm considering writing to libc++ mailing to suggest we move all names into their own headers (e.g. <functional/invoke>, <functional/function>, etc.) and the standard headers just include <__header_name/*> (where * is a placeholder and not what we really include).

I would support such an effort. I'm not exactly sure what granularity we need to reach, however it's clear to me that moving stuff into __functional_base is just a band-aid. We should also split up __functional_base itself.

[tschuett]

@cjdb seemed to be afraid that the two implementations would diverge. I was merely hinting at adding tests as counter-prove. If the solution is macro-foo then I am fine with that.

[ldionne]

I didn't make my "request changes" clear. Is it possible to split it into a header __functional/invoke.h? If not, why?

[• Quuxplusone]

@ldionne wrote:

Is it possible to split it into a header __functional/invoke.h? If not, why?

FWIW, I'd strongly oppose that. If we're going to start introducing dependencies on C++17 invoke (where the rest of libc++ right now takes dependencies only on C++11 __invoke), then don't want __invoke and invoke separated from each other at all. If we're going to treat invoke as synonymous with __invoke, then the synonyms belong together in the same header.

Separately, I'm opposed to creating a "second style precedent" of __functional/foo.h when we already have the established style convention of __functional_foo. (But this is trivially solved by calling a hypothetical new header __functional_invoke instead of __functional/invoke.h.)

Pragmatically, __invoke depends on __invoke_return etc., which right now are scattered among __functional_base and __functional_base_03. So moving invoke into a new header outside of __functional_base, without making that new header itself include __functional_base, seems like a relatively major task. (And of course if the new header does depend on __functional_base then you haven't gained anything, relative to this PR as-is.)

[cjdb]

In D99041#2641549, @Quuxplusone wrote:

@ldionne wrote:

Is it possible to split it into a header __functional/invoke.h? If not, why?

FWIW, I'd strongly oppose that. If we're going to start introducing dependencies on C++17 invoke (where the rest of libc++ right now takes dependencies only on C++11 __invoke), then don't want __invoke and invoke separated from each other at all. If we're going to treat invoke as synonymous with __invoke, then the synonyms belong together in the same header.

That has an easy solution: we move the __invoke components of __functional_base[03] to __functional/invoke.h and be done with it.

Separately, I'm opposed to creating a "second style precedent" of __functional/foo.h when we already have the established style convention of __functional_foo. (But this is trivially solved by calling a hypothetical new header __functional_invoke instead of __functional/invoke.h.)

This isn't something that's set in stone; we can change it if it's deemed to be a better approach. I, for one, would very much prefer __functional/..., because it means we can then have editors collapse directories that aren't relevant. Having a dozen __functional_* complicates the top-level directory (especially if you look forward to a world where we also have __type_traits_*, __utility_*, etc., which I'm very much in favour of).

Pragmatically, __invoke depends on __invoke_return etc., which right now are scattered among __functional_base and __functional_base_03. So moving invoke into a new header outside of __functional_base, without making that new header itself include __functional_base, seems like a relatively major task. (And of course if the new header does depend on __functional_base then you haven't gained anything, relative to this PR as-is.)

[tschuett]

I started a private module map in https://reviews.llvm.org/D94924. It reduces the surface of the std module. Splitting large headers into smaller "tail" headers should support this effort.

[cjdb]

rebases to activate CI

[ldionne]

I'm going to approve this because it's necessary for several other code changes, and it would be unreasonable to hold those up for something that we can easily change later.

That being said, we *will* have something along the lines of __functional/invoke.h in the future - we've had several discussions about this and seems we can't all agree on something, so I'm going to break the tie here.

libcxx/include/__functional_base

 inline _LIBCPP_INLINE_VISIBILITY
 void __user_alloc_construct_impl (integral_constant<int, 2>, _Tp *__storage, const _Allocator &__a, _Args &&... __args )
 {
     new (__storage) _Tp (_VSTD::forward<_Args>(__args)..., __a);
 }
 
 #endif  // _LIBCPP_CXX03_LANG
 
+#if _LIBCPP_STD_VER > 14
+
+template <class _Fn, class ..._Args>
+_LIBCPP_CONSTEXPR_AFTER_CXX17 invoke_result_t<_Fn, _Args...>
+invoke(_Fn&& __f, _Args&&... __args)
+    noexcept(is_nothrow_invocable_v<_Fn, _Args...>)
+{
+    return _VSTD::__invoke(_VSTD::forward<_Fn>(__f), _VSTD::forward<_Args>(__args)...);
+}
+
+#endif // _LIBCPP_STD_VER > 14
+
 _LIBCPP_END_NAMESPACE_STD
 
 #endif  // _LIBCPP_FUNCTIONAL_BASE

libcxx/include/concepts

namespace std {
   // [concept.strictweakorder], concept strict_weak_order
   template<class R, class T, class U>
     concept strict_weak_order = see below;
 }
 
 */
 
 #include <__config>
-#include <functional>
+#include <__functional_base>
 #include <type_traits>
 #include <utility>
 #include <version>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 

libcxx/include/functional

bind(_Fp&& __f, _BoundArgs&&... __bound_args)
     typedef __bind_r<_Rp, _Fp, _BoundArgs...> type;
     return type(_VSTD::forward<_Fp>(__f), _VSTD::forward<_BoundArgs>(__bound_args)...);
 }
 
 #endif  // _LIBCPP_CXX03_LANG
 
 #if _LIBCPP_STD_VER > 14
 
-template <class _Fn, class ..._Args>
-_LIBCPP_CONSTEXPR_AFTER_CXX17 invoke_result_t<_Fn, _Args...>
-invoke(_Fn&& __f, _Args&&... __args)
-    noexcept(is_nothrow_invocable_v<_Fn, _Args...>)
-{
-    return _VSTD::__invoke(_VSTD::forward<_Fn>(__f), _VSTD::forward<_Args>(__args)...);
-}
-
 template<class _Op, class _Tuple,
          class _Idxs = typename __make_tuple_indices<tuple_size<_Tuple>::value>::type>
 struct __perfect_forward_impl;
 
 template<class _Op, class... _Bound, size_t... _Idxs>
 struct __perfect_forward_impl<_Op, __tuple_types<_Bound...>, __tuple_indices<_Idxs...>>
 {
     tuple<_Bound...> __bound_;