Page MenuHomePhabricator

[libcxx] Add string_view literals
Needs ReviewPublic

Authored by AntonBikineev on Nov 17 2016, 7:19 PM.

Details

Summary

This is an implementation of Marshall Clow's P0403 paper proposing an addition of user-defined literals for string_view:

inline namespace literals { inline namespace string_literals {
constexpr string_view    operator "" sv(const char* str, size_t len) noexcept;
constexpr u16string_view operator "" sv(const char16_t* str, size_t len) noexcept;
constexpr u32string_view operator "" sv(const char32_t* str, size_t len) noexcept;
constexpr wstring_view   operator "" sv(const wchar_t* str, size_t len) noexcept;
}}

Diff Detail

Event Timeline

AntonBikineev retitled this revision from to [libcxx] Add string_view literals.
AntonBikineev updated this object.
EricWF edited edge metadata.Nov 17 2016, 7:39 PM

Which paper is this implementing?

Also please update the synopsis comment at the top of the header.

include/string_view
749

If this is new to C++17 then the new declarations should be guarded by #if _LIBCPP_VERSION > 14.

754

Please add inline _LIBCPP_INLINE_VISIBILITY to each of these declarations.

AntonBikineev edited edge metadata.
AntonBikineev marked an inline comment as done.
AntonBikineev added inline comments.
include/string_view
749

Eric, I was thinking about it, but the fact that the whole string_view code is not guarded by _LIBCPP_VERSION > 14 (I guess it's because it can be compiled with lower Standards just fine) stopped me from doing that. I'm still not sure though.

EricWF added inline comments.Nov 18 2016, 3:41 AM
include/string_view
749

Understandable. The decision to backport most of string_view was intentional.

However we shouldn't backport these, since the literal suffix is only supported by the compiler in C++17.

AntonBikineev marked an inline comment as done.Nov 18 2016, 4:16 AM
AntonBikineev added inline comments.
include/string_view
749

Agreed

AntonBikineev marked an inline comment as done.

Fixing typos...

Please ping this once the Clang changes have been accepted. I'm just waiting on those to give this the final OK.

Please ping this once the Clang changes have been accepted. I'm just waiting on those to give this the final OK.

I don't have commit privileges to Clang either, so that patch is stuck. Could you commit there please?

Alright. I committed the Clang changes. Just re-building now so I can test this.

include/string_view
780

// _LIBCPP_STD_VER > 14 comment please.

test/std/strings/string.view/string.view.literals/literal1.pass.cpp
16

What's the point of this test that literal.pass.cpp doesn't cover?

test/std/strings/string.view/string.view.literals/literal2.pass.cpp
16

What's the point of this test that literal.pass.cpp doesn't cover?

EricWF added inline comments.Jan 3 2017, 3:16 AM
test/std/strings/string.view/string.view.literals/literal3.pass.cpp
16

You can move this test into literal.pass.cpp but putting it at another function scope.

EricWF added a comment.Jan 3 2017, 3:19 AM

This LGTM. I'll approve once I see the inline comments addressed.

test/std/strings/string.view/string.view.literals/literal.pass.cpp
10

This needs a // XFAIL: clang-3, gcc-4, apple-clang-7, apple-clang-8

@AntonBikineev when will you be able to make he requested changes? I would like to land this ASAP.

@AntonBikineev when will you be able to make he requested changes? I would like to land this ASAP.

Will do that today

@AntonBikineev when will you be able to make he requested changes? I would like to land this ASAP.

Eric, have you looked at commit 7d32d2f5a1f39d4cae9469645faa74b647698001? This functionality has already been implemented by Marshall, we should abandon this patch.

EricWF added inline comments.Apr 12 2017, 11:12 PM
test/std/strings/string.view/string.view.literals/literal2.pass.cpp
16

Ping on this comment.

test/std/strings/string.view/string.view.literals/literal3.pass.cpp
16

Ping on this comment.

@AntonBikineev Yeah, this should be abandoned. Sorry my mistake.