[PATCH] D21459: Implement http://wg21.link/P0254R1: "Integrating std::string_view and std::string"

[Eric Fiselier via cfe-commits]

EricWF added a comment.

The <string_view>` bits LGTM.


================
Comment at: include/string_view:209
@@ +208,3 @@
+	{
+//             _LIBCPP_ASSERT(__len == 0 || __s != nullptr, "string_view::string_view(_CharT *, size_t): received nullptr");
+	}
----------------
I think we have two choices for this diagnostic:

1. Remove it, since it will never work with C++11 constexpr.
2. Only expose the assertion in C++14 or newer.
3. Only may this constructor constexpr in C++14, and keep the assertion.

(3) seems like a Good option to me since offering string_view in C++11 is an extension anyway.

================
Comment at: include/string_view:314
@@ +313,3 @@
+		__other.__size = __sz;
+//             swap( __data, __other.__data );
+//             swap( __size, __other.__size );
----------------
These lines should be removed?

================
Comment at: include/string_view:331
@@ +330,3 @@
+	{
+// 		if (__pos > size())
+// 			throw out_of_range("string_view::substr");
----------------
These lines should be removed.

================
Comment at: include/string_view:355
@@ +354,3 @@
+	_LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
+	int compare(                       size_type __pos1, size_type __n1, 
+				basic_string_view _sv, size_type __pos2, size_type __n2) const
----------------
Weird whitespace.


https://reviews.llvm.org/D21459