[libcxx-commits] [PATCH] D117400: [libc++] [test] Refactor iterators.common/ctor.pass.cpp

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp:1
 //===----------------------------------------------------------------------===//
 //
----------------
While we're at it, would you mind splitting the tests for these various constructors?

And asking for that made me realize that we're not testing the default constructor at all (we're checking `is_default_constructible` but we're not actually running it).


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp:34
+  static_assert( std::is_default_constructible_v<std::common_iterator<int*, sentinel_wrapper<int*>>>);
+  static_assert(!std::is_default_constructible_v<std::common_iterator<cpp17_input_iterator<int*>, sentinel_wrapper<cpp17_input_iterator<int*>>>>);
 
----------------
See, now you understand why I fixed `cpp17_input_iterator` to *not* be default constructible! 😄


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp:75-76
+    using DerivedIt = std::common_iterator<Derived*, const Derived*>;
+    static_assert(std::is_convertible_v<DerivedIt, BaseIt>); // Derived* to Base*
+    static_assert(!std::is_constructible_v<DerivedIt, BaseIt>); // Base* to Derived*
+    Derived a[10];
----------------
`is_convertible` and `is_constructible` are different in that `is_convertible` won't work if the conversion/construction is explicit. I think this would be better:

```
static_assert( std::is_convertible_v<DerivedIt, BaseIt>); // Derived* to Base*
static_assert(!std::is_convertible_v<BaseIt, DerivedIt>); // Base* to Derived*
```

Unless that isn't what you want (in which case I'd like to understand)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117400