RE: [PATCH] Implement P0966 std::string::reserve should not shrink
It's been a while, but thought I'd check in again now that GCC 9 has been
branched, and master is now on GCC 10.
I've merged my changes with the latest code and attached a diff... Let me know
if there's any thoughts on this.
Thanks,
-Andrew
-Original Message-
From: gcc-patches-ow...@gcc.gnu.org On Behalf
Of Andrew Luo
Sent: Wednesday, January 23, 2019 5:15 PM
To: Jonathan Wakely
Cc: libstd...@gcc.gnu.org; gcc-patches@gcc.gnu.org
Subject: RE: [PATCH] Implement P0966 std::string::reserve should not shrink
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
Sent: Wednesday, January 23, 2019 3:26 AM
To: Andrew Luo
Cc: libstd...@gcc.gnu.org
Subject: Re: [PATCH] Implement P0966 std::string::reserve should not shrink
On 23/01/19 00:44 +, 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
>g...@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 +, 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
RE: [PATCH] Implement P0966 std::string::reserve should not shrink
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
Sent: Wednesday, January 23, 2019 3:26 AM
To: Andrew Luo
Cc: libstd...@gcc.gnu.org
Subject: Re: [PATCH] Implement P0966 std::string::reserve should not shrink
On 23/01/19 00:44 +, 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
>g...@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 +, 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 +, 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.
Rig
