[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.

Dumb question: is there a way to see a list of unresolved comments? All I can
see how to do is look at _all_ the comments in the diff view, which is hard
to parse when there are a lot of resolved ones. I'm not 100% sure I addressed
all of your comments.



================
Comment at: libcxx/include/type_traits:3719
+  template <typename _Tp>
+  static void Accept(_Tp);
+
----------------
philnik wrote:
> 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.
Ah right, thanks. I forgot about the preprocessor!


================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_invocable_r.compile.pass.cpp:83
+// initialize a CantMove object.
+CantMove cant_move = MakeCantMove();
+
----------------
philnik wrote:
> 
It's this way on purpose: this is showing that it's possible to use the
function to initialize a CantMove, i.e. no move constructor is necessary for
such a result type when invoking the function.


================
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)>);
----------------
philnik wrote:
> Could you also add all the tests for `is_invocable_r`?
I assume you don't mean _all_ the tests, right? That seems unnecessary; we're
a lot more likely to have bugs and inconsistencies in the two sets of tests
than for the two implementations to be out of sync. I added a test confirming
that it exists and works. PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125300