Page MenuHomePhabricator

Fix implementation of P0966 - string::reserve Should Not Shrink
Needs ReviewPublic

Authored by andrewluo on Jan 20 2019, 6:02 PM.

Details

Diff Detail

Event Timeline

andrewluo created this revision.Jan 20 2019, 6:02 PM
EricWF added inline comments.Jan 20 2019, 8:55 PM
include/string
966–967

This change is ABI breaking.

Previously reserve was explicitly instantiated in the dylib. After this change it is not.
I'll think more about what the best way to handle this is.

3168

I think we need to re-implement shrink_to_fit after this change, since it is currently implemented as reserve(0).

3168

Since the new behavior is also allowed prior to C++20, I wonder if it would be better to make this change unconditionally?

andrewluo marked 3 inline comments as done.Jan 20 2019, 11:21 PM
andrewluo added inline comments.
include/string
966–967

One possibility would be to introduce a new define, for example _LIBCPP_INLINE_VISIBILITY_AFTER_CXX17. libc++.so is compiled in C++11 dialect so these symbols would be included for pre-C++17, while those setting dialect C++2a (or newer) would get the inline version.

But if we eliminate the #if altogether then this would go away.

3168

Good catch with shrink_to_fit. I will fix that.

I was considering that as well.

My only concern was that older applications that may rely on using reserve to set the capacity (rather than using shrink_to_fit as is proper in C++20) might be affected by this change (behavior changes are more expected when you change the -std dialect options). I'm not too concerned about this, but I'm not familiar with libc++ development practices.

If the consensus is that this is fine, I can make that change. This also solves the issue with changing the ABI by making the function inline.

andrewluo updated this revision to Diff 182810.Jan 21 2019, 9:50 AM
andrewluo marked 3 inline comments as done.

Fixed with the new diff

EricWF added inline comments.Jan 21 2019, 4:39 PM
include/string
966–967

Libc++ may not be compiled in the C++11 dialect. We shouldn't depend on that being the case.

What I think we want to do is something like this:

#ifndef _LIBCPP_BUILDING_LIBRARY
#define _LIBCPP_INLINE_VISIBILITY
#endif
void reserve(size_type __capacity_arg);
967

This needs an _LIBCPP_INLINE_VISIBILITY because we don't want this symbol going into the dylib. That would break backwards compatibility issues.

974

I think we should make this reserve call act the same as it did prior to C++2a. That is, it should call __request_capacity(0) or shink_to_fit()

3153

After some more thinking, I think it's best we leave the old behavior as-is. All of the existing standard libraries seem to honor the shrink request prior to C++20, and that provides mostly consistent behavior to users. For consistency alone I think it's probably better not to backport this extension.

andrewluo updated this revision to Diff 182844.Jan 21 2019, 7:36 PM
andrewluo marked 4 inline comments as done.
andrewluo updated this revision to Diff 183019.Jan 22 2019, 6:56 PM

Made some changes to keep behavior the same in older -std= dialects. We shouldn't overload the function to avoid code such as:

auto f(&::std::string::reserve); // Ambiguous post C++17

from breaking for std < 17 (will break if -std=c++2a however)

Made some changes to keep behavior the same in older -std= dialects. We shouldn't overload the function to avoid code such as:

auto f(&::std::string::reserve); // Ambiguous post C++17

from breaking for std < 17 (will break if -std=c++2a however)

Users are not allowed to take the address of standard library member functions.
See [member.functions]/2 for a description of why - the function you want to take an address of may not exist.

You are right, it is not guaranteed per the standard, although it has worked before (at least on MSVC). Unfortunately, I've seen many instances of code that relies on this implementation-specific behavior (particularly older code that uses bind instead of lambdas).

Anyways though, if you think we should go ahead and split the functions for all dialects anyways (C++2a as well as pre-C++2a) then I can revert that part of the changes...

jwakely added inline comments.
include/string
3153

For libstdc++ I think we have no real choice but to change it for all dialects. Our std::string::reserve(size_type) is exported from the shared library, so users will always get whatever behaviour the library exports, irrespective of their -std options. So we'll be changing reserve(size_type) to never shrink, unconditionally. So the consistency argument is lost.

You are right, it is not guaranteed per the standard, although it has worked before (at least on MSVC).

But not with libc++, where there were already two overloads. Changing that to support broken code which was already unsupported seems unnecessary.

You are right, it is not guaranteed per the standard, although it has worked before (at least on MSVC).

But not with libc++, where there were already two overloads. Changing that to support broken code which was already unsupported seems unnecessary.

Not in any released version of libc++, only after this change (which was made recently): https://reviews.llvm.org/D54992?id=175629

andrewluo marked an inline comment as done.Jan 23 2019, 10:28 AM
andrewluo added inline comments.
include/string
3153

Is it not possible to do the same as here in libstdc++:

Old version of reserve(size_type) is instantiated in the library (thus old code will still receive the old behavior) while marking the function inline for C++2a and forward (thus newer code will see the newer behavior)?

andrewluo updated this revision to Diff 183432.Jan 24 2019, 4:13 PM

Split reserve again for all dialects

andrewluo marked an inline comment as done.Jan 24 2019, 4:14 PM
andrewluo marked an inline comment as not done.

Just wanted to check to see if there are any other comments or if this looks good.