[libcxx-commits] [PATCH] D101003: [libc++] <span>, like <string_view>, has no use for debug iterators.

Wed Apr 28 12:38:15 PDT 2021

Quuxplusone added a comment.

In D101003#2723284 <https://reviews.llvm.org/D101003#2723284>, @ldionne wrote:

> Sorry, been sitting on this patch for a while. I looked into the archives and found a discussion back in August 2019 between @EricWF, @mclow.lists  and I about fixing that very issue, but we never actually did it. At that time I was okay with breaking the span ABI, but that was almost 3 years ago. The other thing that was in the balance at that time was being able to implement `constexpr` operations, but I think we solved that problem with `_LIBCPP_CONSTEXPR_IF_NODEBUG`.
>
> I agree with you in principle and I'm not sure why we have debug iterators in `span` in the first place. On the other hand, this is an ABI break on a feature that we've been shipping. It's a widely used feature, too, so it [has the potential to] impact users. Let's try to quantify how bad it would be.

Okay. But remember, through all of this, we //know// that //nobody// is using `span::iterator` **//in _LIBCPP_DEBUG mode//**, because that doesn't compile. The ABI breakage we're worrying about is ABI breakage specifically in the regular non-debug mode.

> 1. If someone has `span::iterator` as part of a function signature, it'll be a linker error since the mangling will change (from `__wrap_iter` to `T*`).

Agreed. Would you prefer to make `span` use `__wrap_iter` conditionally? I.e.,

  #if _LIBCPP_DEBUG == 2
      using iterator = pointer;
  #else
      using iterator = __wrap_iter<pointer>;  // to maintain our ABI
  #endif

(Again, we know it's safe to change the behavior in `_LIBCPP_DEBUG=1` mode, because there is no behavior there today — it simply doesn't compile.)

> 2. This changes the triviality of `span::iterator`, which is good for performance but also changes how it is handled by the calling convention.

No, `__wrap_iter<T*>` is already trivially copyable today (outside of debug mode).
So I don't think the calling convention would change at all. Example: https://godbolt.org/z/do8o4Ez6z

> 3. Otherwise, the layout of `__wrap_iter<T*>` and just `T*` are the same, so I don't foresee any issues with someone holding a `span::iterator` as a class member or something like that.

Agreed.

@ldionne, I still think we should just break ABI — stylistically nobody //should// ever be passing `span::iterator` across an ABI boundary //period//, which means nobody's writing functions taking `span::iterator`, which means nobody cares about the mangling of those functions' names. (That is, we're not breaking the ABI of `span`; just of its iterators.) Also, better to break early (before claiming C++20 support) than late; if we don't break ABI now I think we're just kicking the tech-debt can down the road to where it will be even //harder// to break ABI in the future. However, I do think the solution I put in response to #1 would be OK if you really wanted to kick that can.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101003