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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

In D130538#3678928 <https://reviews.llvm.org/D130538#3678928>, @philnik wrote:

> Are you absolutely certain that this won't break anything else? I'm definitely not, so I'd say rather break invalid code than valid code.

I'm not absolutely certain, but the same can be said of any change of non-trivial complexity. Even completely ignoring the MySQL issue, this change is arguably an improvement because it brings this implementation closer to how the actual C++20 `iter_move` is specified, and at the end of the day we are trying to emulate that. I believe we would have used `decltype` here from the start, but we had the (incorrect) assumption that it's not supported in C++03. If you have a specific concern, I'm of course happy to address it.



================
Comment at: libcxx/include/__algorithm/iterator_operations.h:84-85
+      __move_t<_Iter> >
+  __iter_move(_Iter&& __i) {
     return std::move(*std::forward<_Iter>(__i));
   }
----------------
ldionne wrote:
> (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.
Done (with a slight tweak to the wording).

I created a helper function and added this assertion to both specializations of `__iter_move`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130538