[libcxx-commits] [PATCH] D74292: [libcxx] adds [concept.derived]

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added a comment.

One correctness comment and some minor nits



================
Comment at: libcxx/include/concepts:155-158
+template<class Derived, class Base>
+concept derived_from =
+    is_base_of_v<Base, Derived> &&
+    is_convertible_v<const volatile Derived*, const volatile Base*>;
----------------
CaseyCarter wrote:
> miscco wrote:
> > As before, this needs _Uglification.
> > 
> > Also note that `is_base_of_v` directly goes back to `__is_base_of` and you have the chain
> > 
> > __is_base_of -> i_base_of -> is_base_of_v
> > 
> > (See https://github.com/miscco/llvm-project/blob/bc29069dc401572ba62f7dd692a3474c1ead76c9/libcxx/include/type_traits#L1417-L1425)
> > 
> > So throughput would be better it you would again specialize for `__clang__` and use `__Is_base_of` directly
> `__is_base_of(Base, Derived)` is available in all versions of clang/GCC/MSVC that support concepts (https://godbolt.org/z/xfLGbF).
> 
> `__is_convertible_to(From, To)` is available in clang/MSVC versions that support concepts (https://godbolt.org/z/jQEEr5). clang also supports the name `__is_convertible`.
If I understand @CaseyCarter correctly then `__is_convertible_to` is not available with gcc so I guess we should use is_convertible_v


================
Comment at: libcxx/test/std/concepts/lang/derived_from.pass.cpp:22
+
+struct Base2 : private Base1 {};
+struct Derived3 : Base2 {};
----------------
I would suggest a better name maybe `Derived_private` and `Derived_public` instead of Derived1/Base2 same for protected




================
Comment at: libcxx/test/std/concepts/lang/derived_from.pass.cpp:152
+  }
+
+  // From as the return type of a pointer-to-function
----------------
That newline is not consistent with the other sections. That said i personally prefer a newline here.  


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

https://reviews.llvm.org/D74292