Page MenuHomePhabricator

Implement LWG 2946, 3075 and 3076
ClosedPublic

Authored by mclow.lists on Jun 26 2018, 3:25 PM.

Details

Summary

A massive amount of doinking around in <string>.

https://cplusplus.github.io/LWG/issue2946
https://cplusplus.github.io/LWG/issue3075
https://cplusplus.github.io/LWG/issue3076

This is not quite right yet, but I wanted to get it up here for people to look at.

I may have stepped on a bug fix for old versions of gcc in C++03 mode - Eric? [ was introduced in r292830 ]
Stephan - I updated your deduction guide tests now that we implement all of them.

Diff Detail

Event Timeline

mclow.lists created this revision.EditedJun 26 2018, 3:25 PM

Ok, for some reason, the four tests that I *added* didn't get marked as "A"

STL_MSFT added inline comments.Jun 26 2018, 3:56 PM
test/std/strings/basic.string/string.cons/string_view_deduction.fail.cpp
27

Repeated "is", occurs repeatedly in other files.

38

You need to include <string_view>, occurs in other files.

test/std/strings/basic.string/string.cons/string_view_deduction.pass.cpp
33

You technically need <memory> for std::allocator, <type_traits> for std::is_same_v.

test/std/strings/basic.string/string.nonmembers/string.special/swap_noexcept.pass.cpp
38

Weird indentation here - is there a tab instead of spaces? Occurs below.

Remove a bunch of noexcepts that I missed the first time.
Put in an error message to be checked in one of the failing deduction cases that was lacking it.
Address STL's comments.

mclow.lists marked 3 inline comments as done.Jun 26 2018, 5:56 PM

Updated the __is_allocator type trait to work all the way back to C++03
Added a bunch of tests from issue2946.

I think this has all the pieces that it needs now.

mclow.lists added inline comments.Jun 27 2018, 7:21 AM
test/std/strings/basic.string/string.ops/string_rfind/string_size.pass.cpp
161

These tests don't work in C++03; they'll need to be ifdef-ed.

Update the tests from 2946 - they are removed for C++03

mclow.lists marked an inline comment as done.Jun 27 2018, 8:29 AM

LGTM. All comments/questions are just for my education. Other things I did: double-check that you changed all the functions changed by https://cplusplus.github.io/LWG/lwg-defects.html#2946.

include/memory
5647

Sorry -- still not very fluent with how things are done in libc++, but don't we need to guard this based on C++11 at the very least because it's using decltype?

include/string
842

Is there a reason why you use a different enable_if pattern here (as a default argument) and above (as a default template argument)?

1646

You don't need to specify the void in enable_if<__is_allocator, void>::type. There's no harm in specifying it, but I'm curious to know if there's a reason for it?

1779

Wow, it's terrible that we need to write this.

More missed noexcepts.

include/string
279

I think you need to remove this noexcept.

286

Remove noexcept.

293

Remove noexcept.

307

Remove noexcept.

mclow.lists added inline comments.Jun 27 2018, 11:53 AM
include/string
842
1646

Habit, I guess - I always put the type into enable_if, even if I don't use it. Sometimes I use nullptr_t instead of void.

ldionne added inline comments.Jun 27 2018, 12:13 PM
include/string
842

Even after reading the SO question, I think the following would work as well?

template<class _Tp, class = typename enable_if<__can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value, void>::type>
_LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
explicit basic_string(const _Tp& __t);

Again, I'm not suggesting any change, just trying to understand,

1646

Understood.

mclow.lists marked 5 inline comments as done.Jun 27 2018, 1:37 PM
mclow.lists added inline comments.
include/memory
5647

libc++ has an emulation of decltype for C++03, based on typeof. It's not perfect, but it works in a lot of cases.

include/string
842

I tried this locally, and it seems to work. Updated patch incoming.

mclow.lists marked 2 inline comments as done.

Update diff per Louis' suggestion.
Remove noexcepts from the synopsis.

ldionne added inline comments.Jun 28 2018, 11:18 AM
include/string
836

I believe an unwelcome explicit snuck in here.

839

Did you mean to leave those comments there?

856

I think this explicit shouldn't be there, too.

tcanens added inline comments.
include/string
856

This one is explicit in the standard (because it had a default argument: template<class T> explicit basic_string(const T& t, const Allocator& a = Allocator());)

1987

__self_view(__t) is wrong - the wording was intentionally crafted to require the conversion to basic_string_view to be done using copy-initialization. Using direct-initialization can potentially result in different overload resolution results.

ldionne added inline comments.Jun 28 2018, 3:20 PM
include/string
856

Ah, you're right. So basically

template<class T>
explicit basic_string(const T& t,
                      const Allocator& a = Allocator());

is implemented as two overloads

template<class T>
explicit basic_string(const T& t);

and

template<class T>
explicit basic_string(const T& t, const Allocator& a);
mclow.lists marked 5 inline comments as done.Jun 28 2018, 5:46 PM
mclow.lists added inline comments.
include/string
836

Right.

839

Nope. Thanks.

856

Right. We frequently split defaulted allocator ctors.

1987

Nice catch!

mclow.lists marked 3 inline comments as done.

Update in response to comments.

ldionne accepted this revision.Jun 29 2018, 1:36 PM

LGTM

This revision is now accepted and ready to land.Jun 29 2018, 1:36 PM
mclow.lists closed this revision.Jul 2 2018, 4:12 PM

Committed as revision 336132.