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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

In D101729#2776136 <https://reviews.llvm.org/D101729#2776136>, @Quuxplusone wrote:

> In D101729#2776070 <https://reviews.llvm.org/D101729#2776070>, @cjdb wrote:
>
>> In D101729#2774913 <https://reviews.llvm.org/D101729#2774913>, @Quuxplusone wrote:
>>
>>> @ldionne: I recommend my blog post https://quuxplusone.github.io/blog/2021/05/07/std-iterator-as-a-base-class/
>>> and the full discussion in D102657 <https://reviews.llvm.org/D102657>. I'll even volunteer to make the "remove all base classes for iterators and function-objects, and drive-by fix the `reverse_iterator` explosion, under an ABI flag" solution PR; it'll just take a week or two before I'm guaranteed to have time for it.
>>
>> That's what this patch does?
>
> This PR (D101729 <https://reviews.llvm.org/D101729>) currently doesn't do any of the three things I mentioned (remove base classes for iterators

This patch definitely removes `std::iterator` as a base class for all iterator adaptors. Is there some other base class I'm not seeing?

> and function-objects,

Function objects are out of scope.

> fix the `reverse_iterator` explosion,

I don't know what "explosion" you're referring to.

> add an ABI flag). But I'd be happy to commandeer it; is that OK by you?

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.

In D101729#2774481 <https://reviews.llvm.org/D101729#2774481>, @ldionne wrote:

> In D101729#2739518 <https://reviews.llvm.org/D101729#2739518>, @EricWF wrote:
>
>> Removing the `std::iterator` inheritance is potentially ABI breaking. In particular in the case of `reverse_iterator`.
>> If the iterator provided to `reverse_iterator` also inherits from `std::iterator`, then the first member of `reverse_iterator` goes from having an offset of 4 to an offset of 0 bytes
>> (because the compiler couldn't place the same empty base at the same address).
>>
>> I'm not sure how much we care, but I see this being a case that could easily arise in real world code.
>> @ldionne thoughts?
>
> Interesting catch Eric, thanks for chiming in. It looks like `raw_storage_iterator` has the same issue since the first member is a user-provided iterator, but I don't think we care for this one since it's deprecated in C++17 and removed in C++20 anyway (so there's likely no important ABI reliance on this type).
>
> Thinking out loud: Before C++17, this is not an issue because `std::iterator` is there and we inherit from it. In C++17 and above, `std::iterator` is marked as `[[deprecated]]`, so if a user inherits from it, they are getting the deprecation warning and also the potential ABI break in `reverse_iterator`. I see the following solutions:
>
> 1. Don't care about it - it has a small chance to happen, and `std::iterator` is deprecated. If we do this, we should push to actually remove `std::iterator` ASAP, since we wouldn't have this problem if `std::iterator` were removed entirely.
>
> 2. In >= C++17, add `static_assert(!std::is_base_of<std::iterator, _Iter>::value, "Would be an ABI break");` inside `reverse_iterator`. That way, if anyone's using `std::reverse_iterator` with a type that would be potentially impacted by an ABI break, they'd be told at compile-time.
>
> 3. Use:
>
>   class __one_byte { char __byte; };
>   class __empty { };
>   
>   template <class _Iter>
>   class _LIBCPP_TEMPLATE_VIS reverse_iterator
>   #if _LIBCPP_STD_VER <= 14
>       : public iterator<typename iterator_traits<_Iter>::iterator_category,
>                         typename iterator_traits<_Iter>::value_type,
>                         typename iterator_traits<_Iter>::difference_type,
>                         typename iterator_traits<_Iter>::pointer,
>                         typename iterator_traits<_Iter>::reference>
>   #else
>       // We must keep the __t member below at the same offset in all Standards, which requires
>       // some work if _Iter inherits from std::iterator.
>       : private conditional_t<is_base_of_v<iterator<XXXX>, _Iter> && !is_final_v<_Iter>, __one_byte, __empty>
>   #endif
>   {
>   private:
>       /*mutable*/ _Iter __t;  // no longer used as of LWG #2360, not removed due to ABI break
>   
>       static_assert(!__is_stashing_iterator<_Iter>::value,
>         "The specified iterator type cannot be used with reverse_iterator; "
>         "Using stashing iterators with reverse_iterator causes undefined behavior");
>   
>   protected:
>       _Iter current;
>   
>       // ...
>   };
>
> (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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101729