[libcxx-commits] [PATCH] D101729: [libcxx] deprecates `std::iterator` and removes it as a base class

Mon May 24 08:33:46 PDT 2021

Mordante added a comment.

In D101729#2776309 <https://reviews.llvm.org/D101729#2776309>, @cjdb wrote:

> In D101729#2776136 <https://reviews.llvm.org/D101729#2776136>, @Quuxplusone wrote:
>
>> 
>
> Your timeline is far out enough that it stalls other work. @Mordante is blocked by this patch.
> It's also a simple enough thing to resolve (once we've agreed on a direction) that handing it over seems pointless.

I'm not really blocked by this patch, I now have an ugly work-around in my format branch. So I could put that up for review and clean up later. And actually I'm not blocked by the base class but the fact the `back_insert_iterator` isn't default constructible and its `difference_type` is `void`.

>> (1) seems a bit reckless to me. (2) seems too constraining to me - I'm sure a lot of older code in the wild is still using `std::iterator` in ways that are not ABI sensitive, and it would be silly to break their code for that.
>>
>> I think I'd go with (3) if we all agree it solves the ABI issue, since it's not such a big deal for us to do. In the future, I'd consider breaking the ABI of `reverse_iterator` and removing both this workaround and the `__t` workaround at once.
>
> I'll grumblingly go along with (3), but if we're going to consider breaking iterator adaptor ABI at "some point" in the future, why not just rip the band-aid off here and now? It'll immediately simplify the implementation, and I can't see how it would be any less reckless to make this decision in a year's time as opposed to today.

I think it would be nice to consider making ABI v2 the default and move Unstable to v3. But I think this requires some planning and communicating to involved parties upfront. Maybe that becomes moot if https://wg21.link/p2123 "Extending the Type System to Provide API and ABI Flexibility" gets accepted before C++23.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101729