[libcxx-commits] [PATCH] D96235: [libcxx] adds concepts `std::invocable` and `std::regular_invocable`

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/concepts:184
+concept invocable = requires(_Fn&& __fn, _Args&&... __args) {
+  _VSTD::invoke(std::forward<_Fn>(__fn), std::forward<_Args>(__args)...); // not required to be equality preserving
+};
----------------
ldionne wrote:
> CaseyCarter wrote:
> > ldionne wrote:
> > > The Standard seems to say that we should call `invoke` unqualified: http://eel.is/c++draft/concept.invocable#concept:invocable.
> > > 
> > > This seems a bit weird to me - I assume I'm missing some context or mis-reading the paragraph prior to the concept definition?
> > I suspect you're forgetting http://eel.is/c++draft/contents#3 "`meow` really means `::std::meow`". Please don't ask me why the Standard always says `std::move` / `std::forward` or my head will explode.
> > 
> > Source comment: there's a weird mixture of `_VSTD::` and `std::` here - is that intended? (I have a vague notion that `std::` is used only for exceptions that are stable across ABIs or something?)
> Oh, thanks.
> 
> The difference between `_VSTD::` and `std::` is unclear to me as well, to be entirely honest. Eric tried removing `_VSTD::` altogether at some point and the attempt was meant with push back by some people who use the `_VSTD` macro to completely different ends.
> 
> I think you might be mistaking this with the fact that we define exception types outside of the `__1` inline namespace so that they mangle the same as exceptions from libstdc++ (when interop between the two was necessary).
FWIW, I think the difference is purely that `_VSTD::` is `std::__1::`. This is significant in SFINAE-related-mangling contexts, e.g. https://godbolt.org/z/xGbTG7 (which GCC gets wrong anyway).

When I did my big `_VSTD::`-normalization patch series (mainly focused on eliminating unwanted ADL but also normalizing what was already there), the only places using `std::` were `std::type_info` and `std::nothrow_t`, both (I assumed) for ABI reasons.

My personal interpretation of libc++ style is that we use `std::` only in weird historical ABI corner cases; `_VSTD::` for things that would trigger ADL (all function calls taking any parameters, but notably not `declval`); and for everything else we use no qualification. E.g. in this file `_VSTD::is_constructible_v` should be just `is_constructible_v`; and in <chrono> I see four accidental instances of `std::numeric_limits` which should be just `numeric_limits`.

Teeny nit: `equality-preserving` should be hyphenated.


================
Comment at: libcxx/test/std/concepts/callable/invocable.compile.pass.cpp:25
+requires std::invocable<F, Args...> constexpr void
+ModelsInvocable(F, Args&&...) noexcept{};
+
----------------
spurious `;`
FWIW, I'd prefer indenting as

    template<class F, class... Args> requires std::invocable<F, Args...>
    constexpr void ModelsInvocable(F, Args&&...) noexcept { }

or

    template<class F, class... Args>
        requires std::invocable<F, Args...>
    constexpr void
    ModelsInvocable(F, Args&&...) noexcept { }



================
Comment at: libcxx/test/std/concepts/callable/invocable.compile.pass.cpp:113
+        std::chrono::high_resolution_clock().now().time_since_epoch().count());
+    auto D = std::uniform_int_distribution<>();
+    ModelsInvocable(D, G);
----------------
Incidentally, blech. ;) What's the default here — `std::uniform_int_distribution<int>`?


================
Comment at: libcxx/test/std/concepts/callable/regularinvocable.compile.pass.cpp:114
+  // 	models_invocable(D, G);
+  // }
+  return 0;
----------------
I would suggest commenting this back in, since a conforming implementation must accept it.
Ideally `diff regularinvocable.compile.pass.cpp invocable.compile.pass.cpp` should produce only a single line of differences — the name of the constraint.
I might also add a test that verifies that `invocable` and `regular_invocable` subsume each other (i.e., a conforming program cannot rely on `regular_invocable` to be a more specialized constraint than plain `invocable`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96235/new/

https://reviews.llvm.org/D96235