[libcxx-commits] [PATCH] D77961: WIP: Adds `std::convertible_to` to <concepts>

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked 3 inline comments as done.
cjdb added inline comments.


================
Comment at: libcxx/test/std/concepts/lang/convertible_to.pass.cpp:21
+constexpr void NotConvertible() {
+  static_assert(
+      !std::convertible_to<typename Mod1<From>::type, typename Mod2<To>::type>);
----------------
miscco wrote:
> Does that compile without the second string argument? It is used everywhere else int eh test suite even if its empty
C++17 makes this possible. I don't think the empty string literal is necessary, since this is a test for C++20.


================
Comment at: libcxx/test/std/concepts/lang/convertible_to.pass.cpp:146
+  OneWayConvertible<int&, int, Identity, Mod2>();
+  TwoWayConvertible<const int&, int, Identity, Identity>();
+  TwoWayConvertible<const int&, const int, Identity, Identity>();
----------------
miscco wrote:
> Should we move this fundamental checks that do not rely on any Modifier to their own function? These are the most common ones so as a reader it would be easier to check them in isolation and then have the specialized function do the tricky stuff
Yes. Do you mean fold them into a function and then call said function here, or have a references namespace?


================
Comment at: libcxx/test/std/concepts/lang/convertible_to.pass.cpp:218
+
+struct ImplicitlyConstructibleAndConvertible {
+  ImplicitlyConstructibleAndConvertible(float);
----------------
miscco wrote:
> I am wondering whether it is cleaner to define those basic types above and for the constructors and then create all the others by inheritance, e.g:
> 
> ```
> struct ImplicitlyConstructibleAndConvertible : public ImplicitelyConstructible, public Convertible {
> {
>     using ImplicitelyConstructible::ImplicitelyConstructible;
> };
> ```
I was planning to do derived cases in a separate namespace to keep what's being tested very explicit. Do you think I'm being //too// granular?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77961