[libcxx-commits] [PATCH] D102657: [libcxx][ranges] Update `{front_, back_, }insert_iterator` for C++20.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/iterator:770-776
+#if _LIBCPP_STD_VER <= 17
     : public iterator<output_iterator_tag,
                       void,
                       void,
                       void,
                       void>
+#endif // _LIBCPP_STD_VER <= 17
----------------
Mordante wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > cjdb wrote:
> > > > Quuxplusone wrote:
> > > > > https://quuxplusone.github.io/blog/2021/05/07/std-iterator-as-a-base-class/
> > > > It's an ABI break that I'm okay with (since I'm okay with //all// ABI breaks), but we should probably discuss this on the mailing list.
> > > Ah yes I forgot about that discussion. Would it be better to just add a dummy base class that we inherit from in ABI-stable mode after C++17?
> > > Would it be better to just add a dummy base class that we inherit from in ABI-stable mode
> > 
> > Yes, iff that base class is spelled `iterator<output_iterator_tag, void, void, void, void>`. Anything else would be an ABI break.
> > I tentatively suggest making an ABI flag for `_LIBCPP_ABI_NO_EMPTY_BASES` affecting //all// the obsolete empty bases; and rolling `reverse_iterator`'s 2^n explosion into that flag as well.  The reason I'm "tentative" is that I still don't understand what is the point of all these ABI flags —
> > - who is the target audience who is expected to turn them on in production?
> > - how do they control which flags they turn on?
> > - how are these flags tested by the buildbots?
> I'm also in favour of adding an ABI flag.
> * Actually I don't expect a lot of customers using the unstable ABI. But we could discuss whether we make the unstable ABI ABI v2 and plan how to roll that out to our customers.
> * The customer can use the `LIBCXX_ABI_UNSTABLE` CMake flag to enable the unstable ABI, or define the individual flags manually.
> * The UBSAN build bot tests it. Recently I was considering to create a buildbot for it, but I noticed we already have one.
> who is the target audience who is expected to turn them on in production?

🤷 Jason Turner noted that in his (relatively biased) survey on breaking ABI, no one seemed to flag that ABI stability was important to them, //personally// ([[ https://youtu.be/By7b19YIv8Q | link to video ]]).

Perhaps we should survey our users and find out

* how many of them genuinely care about ABI stability for projects they own
* which C++ standard they're currently using
* which version of LLVM they're currently using
* when they plan to upgrade LLVM and which version they're planning to upgrade to
* when they plan to move to a newer C++ standard and which they plan to move to

> how do they control which flags they turn on?

I'd say that users who care about ABI stability should enable macros that they care about. Unlike the nodiscard discussion, ABI stability is something that should be opt-in, because it lets us conform to the latest standard while also preserving something that "users" supposedly care about.

Perhaps `_LIBCPP_ABI_NO_CXX17_EMPTY_ITERATOR_BASE` for this one, and `_LIBCPP_ABI_NO_EMPTY_BASES` as a general control mechanism for all the empty base deletions. We could also have `_LIBCPP_NO_CXX17_ABI_BREAK` (to be bikeshedded) that controls all `_LIBCPP_ABI_NO_CXX17_*` macros, in case users want to preserve stability with C++14. This idea is far from fleshed out, and deserves more discussion on Discord.

> how are these flags tested by the buildbots?

I think we can do this with more fine grained macros and libcxx tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102657