[libcxx-commits] [PATCH] D77961: [libcxx] adds concept `std::convertible_to`

[Louis Dionne via Phabricator via libcxx-commits]

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

This looks pretty good to me except the duplication of implementations when intrinsics are available.

More generally, have we looked into the MSVC tests for concepts? Would it make sense to import them here too or at least make sure we have the same coverage?



================
Comment at: libcxx/include/concepts:165
+concept convertible_to =
+  __is_convertible_to(_From, _To) &&
+  requires(add_rvalue_reference_t<_From> (&__f)()) {
----------------
As discussed offline, I would rather we use the same implementation all the time to simplify things. Let's not duplicate each implementation based on whether we can use intrinsics or not. The type traits themselves use the intrinsics, I think that's sufficient.


================
Comment at: libcxx/test/std/concepts/lang/convertible_to.pass.cpp:13
+// template<class From, class To>
+// concept convertible_to;
+
----------------
zoecarver wrote:
> All tests should include "test_macros.h"
Technically, only include `test_macros.h` if you need something defined in it. The one header we always want to include is the one with nasty macros in it, but it's automatically included by the test suite harness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77961