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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

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'll impact users. Let's try to quantify how bad it would be. The issues I can spot are:

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*`).
2. This changes the triviality of `span::iterator`, which is good for performance but also changes how it is handled by the calling convention. So if it gets returned (or passed), it'll go into registers instead of on the stack. That's a pretty major change that would only get detected at runtime if someone's returning those from a function.
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.

This is a tough call, really. To be conservative, I'd say "we can't do that", but I agree things are pretty badly broken if we don't do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101003