[v3 PATCH] Implement std::string_view and P0254r2, Integrating std::string_view and std::string.

Jonathan Wakely jwakely@redhat.com
Wed Jul 27 16:53:00 GMT 2016


On 25/07/16 00:41 +0300, Ville Voutilainen wrote:
>Changelog as it was before, tested on Linux-x64.
>
>I haven't fixed all section references of string tests, and I haven't
>added section references
>to string_view tests, because they didn't have any. I also haven't
>added tests that test the various
>exception-throwing cases for the various operations. Doing those changes is
>a) going to change many many files
>b) probably not urgent
>so here's a modest suggestion:
>let's put this patch in, and tweak the section references in the other
>tests later with separate patches,
>as well as add more comprehensive exception-tests.
>
>Ok for trunk?

+#if __cplusplus > 201402L
+      /**
+       *  @brief  Append a string_view.
+       *  @param __sv  The string_view to be appended.
+       *  @return  Reference to this string.
+       */
+      basic_string& append(__sv_type __sv)
+      {
+       return this->append(__sv.data(), __sv.size());
+      }

One-liners like this don't need braces on a separate line:

      { return this->append(__sv.data(), __sv.size()); }

But they do need a blank line after them before the next function:

+      /**
+       *  @brief  Append a range of characters from a string_view.
+       *  @param __sv  The string_view to be appended from.
+       *  @param __pos The position in the string_view to append
from.
+       *  @param __n   The number of characters to append from the
string_view.
+       *  @return  Reference to this string.
+       */
+      basic_string& append(__sv_type __sv,
+                          size_type __pos, size_type __n = npos)
+      {
+       return _M_append(__sv.data()
+                        + __sv._M_check(__pos,
"basic_string::append"),
+                        __sv._M_limit(__pos, __n));
+      }
+#endif // C++17
+


The Doxygen @file comment in <bits/string_view.tcc> refers to
"experimental" still.

We need to close the versioned namespace before this:

+  namespace __detail
+  {

And reopen it afterwards. i.e.

_GLIBCXX_END_NAMESPACE_VERSION
  namespace __detail
  {
  _GLIBCXX_BEGIN_NAMESPACE_VERSION

  ...

  _GLIBCXX_END_NAMESPACE_VERSION
  }
_GLIBCXX_BEGIN_NAMESPACE_VERSION

Otherwise for the versioned namespace config we would have
std::__7::__detail and also std::__detail::__7 and thigns would get
ambiguous.

I don't think we want the non-standard literals in namespace std:

+  // I added these EMSR.
+  inline namespace literals
+  {
+  inline namespace string_view_literals
+  {
+


I've recently discovered that the order of dejagnu directives matters,
and this is wrong:

+// { dg-do run { xfail *-*-* } }
+// { dg-options "-std=gnu++17 -O0" }
+// { dg-require-debug-mode "" }

It should be dg-options first, then dg-do, then any dg-require-xxx
last. It doesn't actually make any difference in any of the tests in
your patch though, so don't worry about changing it.

The case where it matters is where the dg-do directive has a target
which might be dependent on the dg-options:

// { dg-do compile { target c++1z } }
// { dg-options "-std=gnu++1z" }

This wouldn't get run, because the effective target is tested before
-std=gnu++1z is added to the options.

And this would always run, ignoring the dg-require:

// { dg-require-effective-target c++1z }
// { dg-do compile }

But as I said, don't worry about changing them.

OK for trunk with the changes to formatting, namespaces and literals.

Thanks for doing this!



More information about the Gcc-patches mailing list