RE: [PATCH] Implement P0966 std::string::reserve should not shrink

[Andrew Luo <andrewluotechnologies at outlook dot com>]

Here's an updated diff.  Still a work in progress, ran the tests for strings/capacity in both modes, passing now.  I am running all the tests in both modes as well, but will take a while to get results.  In the meanwhile I thought I'd send this out...

Also, that code works on all released versions of libc++.  Only in the past month or so was that overload added, also for a (slightly flawed) implementation of P0966.  Anyways, I removed the #ifs - I get your point that there's no need to preserve backwards compatibility with non-comforming code, and I think any issues with reserve specifically will be rather rare (and easy to fix).  (Although, I should point out that push_back is not overloaded if you select -std=c++98, only -std=c++11 or later, but that's besides the point).

Thanks,

-Andrew

-----Original Message-----
From: Jonathan Wakely <jwakely@redhat.com> 
Sent: Wednesday, January 23, 2019 3:26 AM
To: Andrew Luo <andrewluotechnologies@outlook.com>
Cc: libstdc++@gcc.gnu.org
Subject: Re: [PATCH] Implement P0966 std::string::reserve should not shrink

On 23/01/19 00:44 +0000, Andrew Luo wrote:
>Thanks for the input.  I will make the suggested changes.
>
>I didn't add a changelog entry yet (which I was planning to do - just wanted to send this out before to get some feedback on the code first).

Understood, I'm just making sure you're aware of the process.

>I will also take care of the copyright assignment (should I send an email to gcc@gcc.gnu.org or are you the maintainer that is taking care of this contribution?)...

That will probably be me. I'll send you the form to start the copyright assignment.

>Next patch I will send to both libstdc++ and gcc-patches as well.  Somehow I missed that.

Thanks.

>One possibility to avoid changing the behavior for pre-C++2a code is to put the check in a "#if __cplusplus > 201703L" block and force reserve(size_type) to be inline (if we want to go this route, I think the helper function _M_request_capacity would have to stay, as reserve could shrink in C++17 mode or earlier). 

>Other than that, with my current patch, reserve() still works as previously, however reserve(0) has changed (I don't know how common it was to use reserve(0) instead of simply reserve())...

Well as you noted, your patch made it a no-op (more below).


>As for the split of reserve - wouldn't this kind of code break in previous -std modes:
>
>auto f(&::std::string::reserve); // Ambiguous post C++17

Already undefined, as you noted in the later reply. More on that below.


>As for deprecation, I will add it to the no-arg overload.  Regarding giving the deprecation notice for all dialects, technically speaking, it was only deprecated in C++2a so should someone receive that deprecation message if they are compiling with -std=c++98, for example?  I don't know what the general policies around this are for GCC/libstdc++.

Yes, you're right it's only deprecated for C++2a. My thinking was that if it's going away at some point in future then it's useful to get a warning saying so, period.

But we don't do that for std::auto_ptr in C++98 even though it's deprecated in C++11. So maybe only add the attribute for C++2a, which means it can use C++11 attribute syntax:

#if__cplusplus > 201703L
    [[deprecated("use shrink_to_fit() instead")]] #endif


On 23/01/19 03:41 +0000, Andrew Luo wrote:
>Actually, reserve() doesn't currently work as reserve(0) previously, I 
>missed a line there (should have shrink_to_fit() in the body...)

Oh, I missed that the proposal says the new overload is a shrink-to-fit request rather than no-op.

Calling shrink_to_fit() won't work for C++98 mode, because it isn't declared. It could be a no-op for C++98 (it's only a non-binding request after all), or we could add a new _M_shrink function that
shrink_to_fit() and reserve() call.  I'm still reluctant to add a new exported function (which means four new symbols) for a deprecated feature that already exists as shrink_to_fit() but maybe that's the best option.

>Also, how do you run the tests for the COW string?  I ran make check in the libstdc++-v3 folder and everything passes...

make check RUNTESTFLAGS=--target_board=unix/-D_GLIBCXX_USE_CXX11_ABI=0

And you can add other options, e.g. to check in C++98 mode:

RUNTESTFLAGS=--target_board=unix/-D_GLIBCXX_USE_CXX11_ABI=0/-std=gnu++98

See https://gcc.gnu.org/onlinedocs/libstdc++/manual/test.html#test.run
for more info.

On 23/01/19 06:17 +0000, Andrew Luo wrote:
>Minor correction, someone kindly pointed out to me elsewhere that this code:
>
>auto f(&::std::string::reserve); // Ambiguous post C++17
>
>was never conforming to the C++ standard in the first place... as splitting the members was legal pre-C++2a regardless.

Right.

>That said, that piece of (non-conforming) code did work before (at least in MSVC + Dinkumware, I didn't check G++ + libstdc++ just yet, but unless G++ has some special enforcement of this in the compiler, it would seem to "work" from the library perspective...).

It won't compile with libc++ though, so it's already non-portable.

>I guess the question how much backward compatibility we want to have with non-conforming code such as the above.  (Personal anecdote - I have seen this kind of stuff in older code, especially when used with bind and mem_fn).

That kind of code breaks routinely, e.g. using &std::vector::push_back which was a single function in C++98 and overloaded in C++11.

Also, such use is incompatible with the always_inline attribute, so if we did try to have different behaviour for older standards, using it via a pointer to member would not necessarily get the C++98 semantics anyway. So I don't think we can realistically retain the old behaviour, we just have to change it unconditionally. We could keep a single overload just to make &string::reserve compile, but I don't think that's worth giving up the chance to streamline
reserve(size_type) by removing the shrinking code path.

>But if you prefer, I can get rid of the #ifs.

Yes please.

Attachment: gccdiff2.txt
Description: gccdiff2.txt