[libcxx-commits] [PATCH] D125300: type_traits: rewrite is_invocable_r in terms of argument passing.

[Aaron Jacobs via Phabricator via libcxx-commits]

jacobsa added a comment.

@philnik: thanks for the quick review! Sorry if I've made any dumb mistakes
here; this is my first time contributing and I'm not familiar with the style,
processes, or tools.

I've left is_nothrow_invocable_r out of this PR to keep it as small as
possible. After learning from this one, the next will require fewer comments.
:-)

By the way, do you know what's up with the failed AIX build? It seems like this
is a pre-existing failure; is it common for that to happen?



================
Comment at: libcxx/include/type_traits:3713
+// REQUIRES: _From is not cv void.
+template <typename _From, typename _To>
+struct __is_implicitly_convertible : is_convertible<_From, _To> {
----------------
philnik wrote:
> This seems more in line with how `invoke_result` is used.
Sorry, I'm not sure what you mean here; could you elaborate? "Convert from
_From to _To" seems like a natural order, and matches is_convertible.


================
Comment at: libcxx/include/type_traits:3714
+template <typename _From, typename _To>
+struct __is_implicitly_convertible : is_convertible<_From, _To> {
+private:
----------------
philnik wrote:
> Why are you inheriting from `is_convertible` and then overwrite all the members?
Oops, this was an editing mistake. Removed; thanks for catching.


================
Comment at: libcxx/include/type_traits:3719
+  template <typename _Tp>
+  static void Accept(_Tp);
+
----------------
philnik wrote:
> You have to _Uglify all the names. These functions shouldn't be static. They should never be instantiated anyways. Could you rename this to `_IsImplicitlyConvertible` or something similar? That would remove the need for the comment above since the code is self explanatory.
I've uglified the names. Can you tell me why it's necessary to do so? I've
never understood this, since we're already in the scope of an internal struct.

I'm not sure these can/should be non-static, since it messes with the ability
to have a constant expression for the decltype tests below, and requires them
to be a terrible mouthful. See below, which also doesn't work for a reason I
haven't dug into. static seems preferable to this. Do you think this is
important?

    template <class _Tp, class = decltype(std::declval<__is_implicitly_convertible>()
                                              .template _RequireImplicitlyConvertibleTo<_To>(_Make<_Tp>()))>
    true_type _TestImplicitConversion(int);
    
    [...]
    
    using type = decltype(std::declval<__is_implicitly_convertible>().template _TestImplicitConversion<_From>(0));



================
Comment at: libcxx/include/type_traits:3721-3723
+  // A function that creates a value of type _Tp.
+  template <typename _Tp>
+  static _Tp Make();
----------------
philnik wrote:
> Couldn't you just use `std::declval`?
That results in an rvalue reference, so it doesn't work correctly for
non-moveable types. The point is to lean on guaranteed copy elision to give the
right answer for those types, matching the standard which defines this in terms
of initialization from an expression of type _From, not _From&&.


================
Comment at: libcxx/test/libcxx/type_traits/is_invocable_r.pass.cpp:2
+// UNSUPPORTED: c++03, c++11, c++14
+
+#include <type_traits>
----------------
philnik wrote:
> You should also put the synopsis here.
Done, although I'm not sure if there is a standard form of synopsis. I've put
something very brief here, because it seems like it's self-explanatory from the
name of the file.


================
Comment at: libcxx/test/libcxx/type_traits/is_invocable_r.pass.cpp:93-97
+///////////////////////////////////
+// main function
+///////////////////////////////////
+
+int main(int, char**) { return 0; }
----------------
philnik wrote:
> You can make this file a `.compile.pass.cpp`. Then you don't have to add a `main`.
Ah, nice tip. Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125300