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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101729