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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

You can also put `is_nothrow_invocable_r` in this PR if you want. The PR is small enough.



================
Comment at: libcxx/include/type_traits:3691-3712
+// Detects whether an express of type _From can be implicitly converted to _To
+// according to [conv]. In C++17, [conv]/3 defines this as follows:
+//
+//     An expression e can be implicitly converted to a type T if and only if
+//     the declaration T t=e; is well-formed, for some invented temporary
+//     variable t ([dcl.init]).
+//
----------------
We normally don't put comments to explain the standard. We have standard for documentation.


================
Comment at: libcxx/include/type_traits:3695-3698
-template <class _Ret, class _Fn, class ..._Args>
-struct _LIBCPP_TEMPLATE_VIS is_invocable_r
-    : integral_constant<bool, __invokable_r<_Ret, _Fn, _Args...>::value> {};
-
----------------
Is there a reason you don't fix `__invokable_r`?


================
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> {
----------------
This seems more in line with how `invoke_result` is used.


================
Comment at: libcxx/include/type_traits:3714
+template <typename _From, typename _To>
+struct __is_implicitly_convertible : is_convertible<_From, _To> {
+private:
----------------
Why are you inheriting from `is_convertible` and then overwrite all the members?


================
Comment at: libcxx/include/type_traits:3718
+  // type T successfully only if T is implicitly convertible to _Tp.
+  template <typename _Tp>
+  static void Accept(_Tp);
----------------
We usually use `class` and not `typename`.


================
Comment at: libcxx/include/type_traits:3719
+  template <typename _Tp>
+  static void Accept(_Tp);
+
----------------
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.


================
Comment at: libcxx/include/type_traits:3721-3723
+  // A function that creates a value of type _Tp.
+  template <typename _Tp>
+  static _Tp Make();
----------------
Couldn't you just use `std::declval`?


================
Comment at: libcxx/include/type_traits:3730
+
+  // A fallback overload selected in all other cases.
+  template <typename _Tp>
----------------
We use these meta programming techniques all over the place. We don't have to document what this does.


================
Comment at: libcxx/include/type_traits:3747
+    // convertible to _Ret.
+    : conditional_t< is_same_v<std::remove_cv_t<_Ret>, void>,
+                     true_type, //
----------------



================
Comment at: libcxx/test/libcxx/type_traits/is_invocable_r.pass.cpp:1
+// UNSUPPORTED: c++03, c++11, c++14
+
----------------
This file is missing the license header. It should also be in `test/std` since it's standard behaviour and nothing libc++ specific.


================
Comment at: libcxx/test/libcxx/type_traits/is_invocable_r.pass.cpp:2
+// UNSUPPORTED: c++03, c++11, c++14
+
+#include <type_traits>
----------------
You should also put the synopsis here.


================
Comment at: libcxx/test/libcxx/type_traits/is_invocable_r.pass.cpp:5-8
+///////////////////////////////////
+// Non-invocable types
+///////////////////////////////////
+
----------------
Same for the other tests


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125300