[libcxx-commits] [PATCH] D130538: [libc++] Fix `_IterOps::__iter_move` returning a dangling reference for non-conforming iterators.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I like this new approach regardless of the issues raised in D129823 <https://reviews.llvm.org/D129823> because it is closer to `decltype(auto)`, and that's what we're trying to achieve at the end of the day.

However, the truth is that any iterator that lies about its `iterator_traits::reference` is a potential issue just waiting to explode, and since we can easily diagnose it, I think we should. That's why I've suggested adding a `static_assert` as a QOI thing.

Is it going to break some code? Probably, but that will actually point out potential bugs in people's code, which is a nice property to have.



================
Comment at: libcxx/include/__algorithm/iterator_operations.h:68
+  template <class _Iter>
+  using __deref_t = decltype(*std::declval<_Iter>());
+
----------------
This way, we'll match the definition of `iter_reference_t`.


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:71
+  template <class _Iter>
+  using __move_t = decltype(std::move(std::declval<__deref_t<_Iter> >()));
+
----------------
IMO that's easier to read, since I don't have to substitute `__deref_t` in my head and work through the additional `declval<T>()` layer.


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:77-80
+  // If the result of dereferencing `_Iter` is a reference type, deduce the result of calling `std::move` on it.
+  // Note that we can't rely on `iterator_traits` because those might be invalid. While that makes the user code
+  // non-conforming, it wouldn't be visible if the implementation used `foo = std::move(*iter);` rather than
+  // `foo = _IterOps<_Policy>::__iter_move(iter)`.
----------------



================
Comment at: libcxx/include/__algorithm/iterator_operations.h:84-85
+      __move_t<_Iter> >
+  __iter_move(_Iter&& __i) {
     return std::move(*std::forward<_Iter>(__i));
   }
----------------
(suggested message, feel free to change)

The full link is http://eel.is/c++draft/input.iterators#tab:inputiterator.

Also, please add a libc++ specific test to ensure that we do trigger the `static_assert` at least when we call `std::sort` with a mis-behaving iterator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130538