[libcxx-commits] [PATCH] D108575: [libcxx] Define insert_iterator::iter with ranges::iterator_t

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb accepted this revision.
cjdb added a comment.

This looks like it's ready to rock as-is. The only comment of mine that's blocking is the one addressing diagnostic concerns.



================
Comment at: libcxx/include/__iterator/insert_iterator.h:57
 
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 insert_iterator(_Container& __x, typename _Container::iterator __i)
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 insert_iterator(_Container& __x, decltype(iter) __i)
         : container(_VSTD::addressof(__x)), iter(__i) {}
----------------
Quuxplusone wrote:
> jloser wrote:
> > @cjdb @ldionne @Quuxplusone this is a drive-by fix so that the constructor of `insert_iterator` matches the iter type declared as the protected member. Do we like using `decltype(iter)` here so it's correct in both cases or would we prefer an `#ifdef` here like we do when defining the `iter` member?
> `decltype(iter)` seems fine to me. (This is a good catch! The new test achieved its goal!)
> Do we like using `decltype(iter)` here so it's correct in both cases or would we prefer an `#ifdef` here like we do when defining the `iter` member?

If this isn't public: have at it. If it is public: please use something that will produce the most meaningful diagnostic to the user.



================
Comment at: libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp:33
+static_assert(std::is_constructible_v<std::insert_iterator<NoIteratorAlias>, NoIteratorAlias&, double*>);
+static_assert(!std::is_constructible_v<std::insert_iterator<NoIteratorAlias>, NoIteratorAlias&, int*>);
+
----------------
Let's use `constructible_from` since it's also got a destructible sanity check.


================
Comment at: libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp:39
+    auto it = std::insert_iterator<NoIteratorAlias>(c, c.data_);
+    ASSERT_SAME_TYPE(decltype(std::inserter(c, c.data_)), std::insert_iterator<NoIteratorAlias>);
+    *it++ = 1 + half;  // test that RHS is still implicitly converted to _Container::value_type
----------------
Quuxplusone wrote:
> jloser wrote:
> > Should we just use `static_assert` with `std::is_same_v` since this test file only supports C++20 anyway (meaning there isn't a need to route through `ASSERT_SAME_TYPE`)?
> I think the question is not quite settled globally, but personally, I prefer `ASSERT_SAME_TYPE` because
> - it is shorter, and
> - it "always works" — doesn't require making any judgment calls about which C++ versions the test supports — and
> - it won't require refactoring five years down the road when the test gets extended to other C++ versions (this happens more often than you might think).
> 
> Hypothetically, if we //were// to go the "always use the latest and greatest possible, no software-engineering considerations" route, then we'd also have to consider `static_assert` with `std::same_as` ("since this test file only supports C++20 with concepts+ranges anyway").
Unless you're intentionally directly testing the constructor on line 38, the way in which I'd write this is
```
std::same_as<std::insert_iterator<NoIteratorAlias>> auto it = std::insterer(c, c.data_);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108575