This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P2499R0 (`string_view` range constructor should be `explicit`)
ClosedPublic

Authored by fsb4000 on Jul 29 2022, 10:00 AM.

Diff Detail

Event Timeline

fsb4000 created this revision.Jul 29 2022, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2022, 10:00 AM
fsb4000 requested review of this revision.Jul 29 2022, 10:00 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJul 29 2022, 10:00 AM
philnik accepted this revision.Jul 29 2022, 10:30 AM
philnik added a subscriber: philnik.

LGTM % nits. Do you need someone to land it? If yes, please provide "Your Name" <your@email.addres>.

libcxx/docs/ReleaseNotes.rst
40
libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp
189

I wouldn't bother saying which paper changed it. It's not like anybody will care when reading this later.

This revision is now accepted and ready to land.Jul 29 2022, 10:30 AM
fsb4000 added a comment.EditedJul 29 2022, 10:32 AM

Yes.

"Igor Zhukov" <fsb4000@yandex.ru>

I will fix the tests.

fsb4000 updated this revision to Diff 448677.Jul 29 2022, 10:41 AM

fix tests

fsb4000 updated this revision to Diff 448678.Jul 29 2022, 10:44 AM

remove mention of paper in tests

fsb4000 updated this revision to Diff 448679.Jul 29 2022, 10:46 AM

remove "Implemented" in ReleaseNotes.rst

fsb4000 updated this revision to Diff 448690.Jul 29 2022, 11:28 AM
fsb4000 marked 2 inline comments as done.

fix std\strings\string.view\string.view.deduct\range.pass.cpp

Mordante accepted this revision.Aug 1 2022, 9:14 AM
Mordante added a subscriber: Mordante.

LGTM modulo one nit.

libcxx/docs/ReleaseNotes.rst
40

This is the style we switched to in the previous release, so let's keep using that.

fsb4000 updated this revision to Diff 449041.Aug 1 2022, 9:23 AM

new style of implemented papers in ReleaseNotes.rst

ldionne reopened this revision.Aug 2 2022, 8:20 AM
ldionne added a subscriber: ldionne.

This was accepted as a paper, not as a LWG issue/DR, right? If so, why is this not guarded with a version check for the C+ Standard?

This revision is now accepted and ready to land.Aug 2 2022, 8:20 AM
fsb4000 marked an inline comment as done.Aug 2 2022, 8:26 AM

Hi Louis Dionne!

This is tested above: https://github.com/llvm/llvm-project/blob/8be1197285bdff6e03bf26b77052daa710f73068/libcxx/include/string_view#L319

#if _LIBCPP_STD_VER > 20 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)

So this is C++23 change.

This was accepted as a paper, not as a LWG issue/DR, right? If so, why is this not guarded with a version check for the C+ Standard?

Both the addition of the constructor itself and this change are part of C++23.

ldionne closed this revision.Aug 2 2022, 10:16 AM

Sorry, I looked way too quickly at this patch. Thanks for the patch and for correcting me!