In D155486#4551579, @philnik wrote:

In D155486#4544305, @Mordante wrote:

In D155486#4532334, @mvels wrote:

BTW: I did not understand the Apple max_size.pass failure, I adjusted the test and they all pass (check-cxx) on my local git repo on x86.

On the backdeployment targets the new code uses the old version of the dylib. My suspicion is there is an ABI incompatibility. @philnik do you see an issue in this patch?

This is an ABI break, but I don't think that's the problem. max_size isn't instantiated. This look to me like there is an off-by-one error somewhere.

Could we split this into two patches? This is really seems like it has high potential for having subtile bugs.

Remove parenthesis from debugging

Fix what I am sure is a bug in checking for max_size() in grow_by_....

BTW: I did not understand the Apple max_size.pass failure, I adjusted the test and they all pass (check-cxx) on my local git repo on x86.

Added extra comments on string growth

Fix headers removed by clang-format mess

Upload correct version this time

Upload grow_by amended changes

Add grow_by without replace

_LIBCPP_ABI_UNSTABLE is meant for improvements (in correctness, performance or other) that require breaking the ABI.

Some 2 cents from my side.

In D102727#2880793, @ckennelly wrote:

In D102727#2880396, @ldionne wrote:

@ckennelly I know you did a lot of work on std::string, can you take a look at this change?

I've added @mvels who has done a lot more looking at std::string than I have.

Sorry, didn't see this earlier as this drowned in my email filters :/

In D81770#2102480, @minjaehwang wrote:

clang-tidy complains that "benchmark/benchmark.h" is not found at CartesianBehcmarks.h. This diff does not modify that file and this is a benign error. It compiles without a problem.

Could anyone land this diff into the master?

In D78763#2104284, @mvels wrote:

I'm going to abandon this diff, seems arcanist, phabricator and I can't see each other eye to eye...... :\ which makes me sad,....

Create new diff (same as this) in https://reviews.llvm.org/D82220

I'm going to abandon this diff, seems arcanist, phabricator and I can't see each other eye to eye...... :\ which makes me sad,....

arc diff with --head=HEAD 'HEAD~1'

Running arc diff --update D78763

Looks like harbormaster builds use arcanist, and arcanist has **magic ** making awful diffs?

I'm mystified why the build fails, it fails on applying the patch:

Uploading clean diff

Uploaded clean diff in case harbormaster is confused about diffs

Thanks for the reviews!

Optimed ternary op for ascizz string -> always evalutate length in inlined constant string.

Cleaned up const char* ternary, evaluating traits_type::length for all constant strings

Removed __config changes

Revamped

Fixed __config comment

Fixed NOEXCEPT comments

renamed end --> pos

Update diff after master pull

Removed Apple V2 abi list

White space / comment clean up

Fixed AsanTransaction for single 'construct_at' use case

Well, I think we should probably remove that notion, or merge it with the "unstable" ABI. It's not really a thing since V2 is basically unstable AFAICT.

Just fix it and we can improve the situation (i.e. probably remove it) later.

I have no way to verify / test this (cross compiling apple darwin v2 looks to be impossible?), and guessing the mangled names from x86 compiles seems dubious as well given the diffs I see on apple v1 vs linux v1

1. Pass this->__end_ to the helper functions
2. Return the new end pointer from those helpers

I am skeptical why step 2 is needed at all. You never remove setting of this->__end_. So why do you need to do work that has already been done? Could you please verify that the second part is indeed necessary?

If it is indeed necessary I note that you pessimize the slow path by decrementing and then incrementing.

I would greatly prefer it if you would directly return in both the fast and the slow path.

They are removed from the unstable ABI, but not from the ABI v2. Those are different things (and I guess it doesn't really make sense for them to be different). But we should either decide that we don't test abi lists for ABI v2, or fix it. I suggest we fix it for now in order to make progress on this. You just need to update the ABI list for v2, it shouldn't be difficult.

Restricted assign to __min_cap

In D78763#2001599, @ldionne wrote:

Please make sure you update the ABI v2 abi list when you make these changes. It's currently broken (and this change will break it even more). You can run make check-cxx-abilist to see the failures.

Or, I guess we can also have a discussion to disable the abi list tests for unstable ABIs, since it's not that useful I guess.

Fixed external instantiations for unstable (removed __resize)

Simplified change to inlined optimizations.

Added proper constant for min short -> long promotion size

Adjusted for sizeof(value_type)

New diff after rebase

Thanks, arc is not my friend :\ Fixed the diff using --base=git:origin

Thanks, arc is not my friend :\ Fixed the diff using --base=git:origin

BTW, you could argue 'arc' was smart enough to recognize this was a rolled forward revert, and only diff against the original submit, alas :)

Diff against