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

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added inline comments.


================
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>();
----------------
cjdb wrote:
> 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?
Given that the namespace is "Fundamentals" I would move it into its own function and test it against some basic types such as `int`, `struct A{};`, `enum class` etc


================
Comment at: libcxx/test/std/concepts/lang/convertible_to.pass.cpp:218
+
+struct ImplicitlyConstructibleAndConvertible {
+  ImplicitlyConstructibleAndConvertible(float);
----------------
cjdb wrote:
> 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?
But you are doing essentially derived cases here. `ImplicitlyConstructibleAndConvertible` is a conjunction of `ImplicitelyConstructible` and `ImplicitelyConvertible`

Note that this is just a question whether it is actually easier to read rather than a correctness issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77961