[PATCH] D152891: [ADT] Add deduction guide for iterator_range

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: llvm/include/llvm/ADT/iterator_range.h:49-51
   iterator_range(Container &&c)
-  //TODO: Consider ADL/non-member begin/end calls.
-      : begin_iterator(c.begin()), end_iterator(c.end()) {}
+      : begin_iterator(adl_begin(std::forward<Container>(c))),
+        end_iterator(adl_end(std::forward<Container>(c))) {}
----------------
steakhal wrote:
> tahonermann wrote:
> > Static analysis flagged this code because `c` is forwarded twice thereby introducing the possibility that the second callee attempts to move from a moved-from object.
> > 
> > In practice, this code is unlikely to be problematic because a container/range that supports moving an rvalue object into the iterator returned by its associated `begin()` and which has an `end()` that is sensitive to the identity or state of the range object would be a very strange thing indeed (I can imagine a range that models an input-range that supports moving a range object into the iterator returned by `begin()`, but then `end()` would almost certainly return a sentinel that is independent of the identity or state of the range object). Still, a double forward like this is quite strange.
> > 
> > There is some offline discussion happening that might result in a code review being submitted to remove one or both of the `std::forward()` calls.
> Makes sense. The intention was to select the right overload, but indeed, its confusing.
> Submit a patch for simply removing both forwards. I don't expect constructing an iterator to suddenly consume the container anyway. It wouldn't make sense.
I've uploaded D157453 to remove the std::forward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152891