[libcxx-commits] [PATCH] D92776: [libc++] ADL-proof <algorithm> by adding _VSTD:: qualification on calls.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/algorithm:1739
         return _VSTD::__copy(
-            __unwrap_iter(__first), __unwrap_iter(__last), __unwrap_iter(__result));
+            _VSTD::__unwrap_iter(__first), _VSTD::__unwrap_iter(__last), _VSTD::__unwrap_iter(__result));
     }
----------------
Quuxplusone wrote:
> rnk wrote:
> > This broke Chrome, apparently we were using this ADL extension point to enable memmove optimization for some fancy iterators:
> > https://source.chromium.org/chromium/chromium/src/+/master:base/containers/checked_iterators.h;l=88?q=__unwrap_iter&ss=chromium
> > 
> > This std::copy optimization was added in April, maybe it's not that important:
> > https://chromium-review.googlesource.com/c/chromium/src/+/1875734
> > 
> > Can you suggest an alternative solution to make std::copy use memmove for some custom iterator?
> Oh, yuck. @ldionne @ericwf might want to take a look.
> As the discussion on [that pull request](https://chromium-review.googlesource.com/c/chromium/src/+/1875734) indicated, C++20 provides a //standard// way to make a contiguous iterator that the library will recognize; however, there are two problems with that approach right now. (1) it's not implemented, and (2) it's not the best-designed, for example it requires you to overload `std::to_address(CCI)` (//not// as an ADL customization point; actually in `std`!).
> I think it would be useful to users if libc++ provided a simple way to mark an iterator as contiguous, even in C++11/14/17. I suggest:
> - libc++ should define the tag type `std::contiguous_iterator_tag` in all language modes, as an extension.
> - libc++ should treat as contiguous, any iterator for which `std::is_base_of_v<std::contiguous_iterator_tag, std::iterator_traits<It>::iterator_category>`.
> - To convert a contiguous iterator `it` to a pointer, libc++ should call `std::to_address(it)` just the way it would in C++20(??).
> 
> I've opened https://stackoverflow.com/questions/65712091/in-c20-how-do-i-write-a-contiguous-iterator to find out more about how users such as Chromium are supposed to do this kind of stuff.
I know @EricWF  had done a lot of work towards enabling this use-case, and I think he had a consistent design in mind. I'd like him to chime in on this issue.

What Arthur says makes sense to me and I'd be in favour of following that path, however like I said I absolutely want to have Eric's thoughts first because he had worked on the chromium optimization in the first place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92776