Thanks for reviewing this @PiotrZSL!

Mention this change in the release notes

I suppose it sounds sensible to have the option of ignoring certain containers in this check; though I haven't needed it myself so far, which is also why I'm leaning against ignoring std::array by default. But I do not claim ultimate authority on this question, of course.

Thanks for your help @philnik. I'm still unsure what exactly I should check for in the exception header; I went for

#if defined(_LIBCPP_ABI_VERSION) || defined(LIBCXXRT) || defined(LIBCXX_BUILDING_LIBCXXABI)

because the first two are used in libcxx/src/exception.cpp to indicate that the exception_pointer_cxxabi.ipp implementation is to be used, and it didn't work without the last one, but it doesn't really look right to me, so please comment.

In D122536#3633419, @philnik wrote:

The problem is that you have introduced new symbols to the dylib. I think the easiest way to avoid this is to use __cxa_{increment, decrement}_exception_refcount in the header directly.

In D122536#3631434, @fwolff wrote:

I think the main problem were the inconsistent abilist files, which I have attempted to fix here

@Mordante I think the main problem were the inconsistent abilist files, which I have attempted to fix here (unfortunately generate-cxx-abilist didn't really work for me, as it created lots of other entries for things I didn't touch, so I had to do it manually). I've also rebased the patch, though the rebase went cleanly.

Thanks for reviewing this @Mordante! I think I've addressed all of your comments, and I've added a suppression for -Wgnu-inline-cpp-without-extern, which will hopefully fix the failing tests.

Ping @aaron.ballman.

In D114119#3449098, @ldionne wrote:

Also, if you re-upload a patch, you should be able to see if it hangs forever (we now enforce a timeout in our CI).

Thanks a lot for your analysis @jiixyj! Your analysis of the ABA problem makes sense to me.

Thanks for your help @philnik. I have now revised my implementation so that the ABI break is no longer required.

In D122536#3445811, @philnik wrote:

Adding the move constructor and assignment operator isn't ABI-breaking AFAICT. IIUC the ABI breaking part of this patch is that the functions don't get generated anymore in the dylib. Using __attribute__((__gnu_inline__)) allows us to suppress generating the functions if they're not inlined, but allows the compiler to inline the function. Now if we don't apply the attribute when building the library the functions will be emitted, but otherwise they won't. This way we have the same linkage and so on for the dylib function, but have all the optimization opportunities of an inline function.

In D122536#3409909, @philnik wrote:

I think this could be implemented in a non ABI breaking way by using __attribute__((__gnu_inline__)). See https://godbolt.org/z/W83rTb714 for an example. If you want to allow it in ABIv1 this way you should probably just disable the magic in ABIv2.

In D122257#3442901, @ldionne wrote:

D122612 is good to go, I think you should be able to rebase onto main soon, apply my remaining comments and then we can ship this!

Thanks for reviewing this @ldionne!

Thanks for the quick review @aaron.ballman! I think I've addressed your comments.

In D114272#3299405, @RKSimon wrote:

@fwolff Please can you rebase this patch?

Fixed the nits. Thanks for the review @aaron.ballman!

Rebased and ping @whisperity

Rebased again. Ping @lebedev.ri

@aaron.ballman Ping? I think I've responded to all comments so far; let me know if you still have concerns.

I've added the static_cast and an entry in the release notes. I'm not sure how to handle [[no_unique_address]], so I'd rather leave this to future work.

In D115124#3209199, @Sockke wrote:

Could you please add a test case where the smart pointer object is dereferenced before calling size()? E.g. return (*ptr).size() == 0;. I think this change doesn't work on this test case.

In D114272#3179252, @lebedev.ri wrote:

@fwolff reverse ping?
This doesn't need any big fundamental changes, just a little rebasing.

In D115125#3172432, @Quuxplusone wrote:

My initial reaction is that this change slows down the common case to cater to a really super pathological case; I don't think we should do it (but a benchmark could probably change my mind).

Thanks for your comments @whisperity! They should all be fixed now.

Thanks for your comments @whisperity. I think I've addressed them, could you have another look?

In D114299#3164127, @aaron.ballman wrote:

Hmm, the test case you added passes without the patch applied: https://godbolt.org/z/T9TerMYGz

In D114129#3165399, @leonardchan wrote:

Is there something wrong on our end that's causing this to happen, or is this from something incorrect with the patch?

In D113828#3164016, @aaron.ballman wrote:

LGTM! Can you add a release note about the bug fix?

In D114129#3162487, @ldionne wrote:

Ok. In that case, can we add a comment somewhere in uniform_int_distribution that __int128_t is supported?

In D114129#3158805, @Mordante wrote:

I prefer to allow this extension; I even wonder whether not allowing 128-bit values was intentionally or an artifact of prohibiting (un)signed char. In general I prefer to allow __[u]int128_t to be used as a first-class citizen in libc++.