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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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> {
----------------
jacobsa wrote:
> 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.
I think it's debatable whether `_From` `_To` is the natural order. I always have to look that up. But I'm Ok with keeping it the same as `is_convertible`. 


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


We have to _Uglify all the names because a User might `#define Make "This is definitely not an identifier"` and it is perfectly legal. We don't have to do the same shenanigans in files that the user will never see the internals of. You might notice that we don't _Uglify all the names in `libcxx/src`. There we only _Uglify the public ones.
So at some point in the future, when modules are used everywhere and the standard doesn't define things in terms of headers anymore we can stop _Uglifying the new code. So we can probably stop in 10 years if we're lucky.

> 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 [...]
Oops, I forgot that they are part of a struct. Ignore that part.


================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp:11
+
+// Tests for is_invocable_r.
+
----------------



================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp:74
+};
+
+static_assert(!std::is_move_constructible_v<CantMove>);
----------------
Could you also add tests with arguments?


================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp:79
+// Define a function that returns that type.
+CantMove MakeCantMove() { return CantMove(); }
+
----------------



================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp:83
+// initialize a CantMove object.
+CantMove cant_move = MakeCantMove();
+
----------------



================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp:91
+// some other type.
+static_assert(!std::is_invocable_r_v<int, decltype(MakeCantMove)>);
----------------
Could you also add all the tests for `is_invocable_r`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125300