This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]