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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 7 inline comments as done.
Quuxplusone added inline comments.


================
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) {}
----------------
cjdb wrote:
> 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.
> 
This is how it looks after the PR:
```
x.cpp:7:39: error: no matching constructor for initialization of 'std::insert_iterator<decltype(v)>' (aka 'insert_iterator<vector<int>>')
    std::insert_iterator<decltype(v)> it(v, w.begin());
                                      ^  ~~~~~~~~~~~~
build2/include/c++/v1/__iterator/insert_iterator.h:58:61: note: candidate constructor not viable: no known conversion from 'std::vector<double>::iterator' (aka '__wrap_iter<double *>') to '__insert_iterator_iter_t<std::vector<int>>' (aka 'std::__wrap_iter<int *>') for 2nd argument
    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 insert_iterator(_Container& __x, __insert_iterator_iter_t<_Container> __i)
                                                            ^
```
compared to before:
```
x.cpp:7:39: error: no matching constructor for initialization of 'std::insert_iterator<decltype(v)>' (aka 'insert_iterator<vector<int>>')
    std::insert_iterator<decltype(v)> it(v, w.begin());
                                      ^  ~~~~~~~~~~~~
build/include/c++/v1/__iterator/insert_iterator.h:52:61: note: candidate constructor not viable: no known conversion from '__wrap_iter<std::vector<double>::pointer>' to '__wrap_iter<std::vector<int>::pointer>' for 2nd argument
    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 insert_iterator(_Container& __x, typename _Container::iterator __i)
                                                            ^
```
I call this "no significant difference in awfulness."


================
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*>);
+
----------------
cjdb wrote:
> Let's use `constructible_from` since it's also got a destructible sanity check.
This assertion is just sanity-checking that `NoIteratorAlias` is not constructible from `int*`, because if it //were// constructible from `int*`, then that would nerf the whole point of the test (which is to verify that `insert_iterator` doesn't accidentally try to construct it from `int*` instead of `double*`). Destructibility is irrelevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108575