diff mbox

Finish implementing P0426R1 "Constexpr for std::char_traits" for C++17

Message ID defced1f-1130-15fb-10dc-98b0eb4645f4@redhat.com
State New
Headers show

Commit Message

Pedro Alves April 23, 2017, 6:33 p.m. UTC 
On 04/23/2017 06:54 PM, Pedro Alves wrote:
> Hi!
> 
> As I had suggested in PR c++/80265, here's a patch that uses
> __builtin_constant_p to tell whether we can defer to a constexpr
> algorithm, which avoids having to wait for compiler support.
> 
> Unfortunately I ran out of cycles today to run a full bootstrap/regtest
> cycle, but constexpr_functions_c++17.cc passes cleanly on
> x86_64 GNU/Linux at least.
> 
> If this looks like a reasonable approach, I can maybe try running
> full tests on the gcc compile farm next week.
> 
> WDYT?

Reading the patch via the list made me spot something
that was incorrect.

>  
> -      static char_type*
> +      static _GLIBCXX17_CONSTEXPR char_type*
>        assign(char_type* __s, size_t __n, char_type __a)
>        {
> +#if __cplusplus > 201402
> +	if (__constant_string_p(__s)

This should probably have been __constant_char_array_p, but now
that I look again at the paper, I see I got myself confused -- this
assign overload is not meant to be constexpr in the first place.

So here's an updated patch that drops that bit.  (Same testing
as before.)

From 71cf9b9a3ea79bd790d792c9a02e5a577f1af156 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sun, 23 Apr 2017 14:16:09 +0100
Subject: [PATCH] Finish implementing P0426R1 "Constexpr for std::char_traits"
 for C++17

As discussed in PR c++/80265 (__builtin_{memcmp,memchr,strlen} are not
usable in constexpr functions), use __builtin_constant_p to tell
whether we can defer to a constexpr algorithm.

I used __always_inline__ just to be thorough.  It isn't really really
necessary as far as I could determine.

Changes like these:

	 if (__n == 0)
	   return 0;
 -	return wmemcmp(__s1, __s2, __n);
 +	else
 +	  return wmemcmp(__s1, __s2, __n);

are necessary otherwise G++ complains that we're calling a
non-constexpr function, which looks like a a manifestation of PR67026
to me.

libstdc++-v3:
2017-04-23  Pedro Alves  <palves@redhat.com>

	* doc/xml/manual/status_cxx2017.xml: Update C++17 constexpr
	char_traits status.
	* doc/html/*: Regenerate.

	* include/bits/char_traits.h (_GLIBCXX_ALWAYS_INLINE): Define if
	not already defined.
	(__cpp_lib_constexpr_char_traits): Uncomment.
	(__constant_string_p, __constant_char_array_p): New.
	(std::char_traits<char>, std::char_traits<wchar_t>): Add
	_GLIBCXX17_CONSTEXPR on compare, length and find and use
	__constant_string_p, __constant_char_array_p and
	__builtin_constant_p to defer to __gnu_cxx::char_traits at compile
	time.

	* testsuite/21_strings/char_traits/requirements/
	constexpr_functions_c++17.cc: Uncomment
	__cpp_lib_constexpr_char_traits tests.  Uncomment
	test_compare<char>, test_length<char>, test_find<char>,
	test_compare<wchar_t>, test_length<wchar_t> and test_find<wchar_t>
	static_assert tests.
---
 libstdc++-v3/doc/xml/manual/status_cxx2017.xml     |   4 +-
 libstdc++-v3/include/bits/char_traits.h            | 101 ++++++++++++++++++---
 .../requirements/constexpr_functions_c++17.cc      |  16 ++--
 3 files changed, 100 insertions(+), 21 deletions(-)

Comments

Jonathan Wakely June 5, 2017, 2:27 p.m. UTC | #1 
On 23/04/17 19:33 +0100, Pedro Alves wrote:
>On 04/23/2017 06:54 PM, Pedro Alves wrote:
>> Hi!
>>
>> As I had suggested in PR c++/80265, here's a patch that uses
>> __builtin_constant_p to tell whether we can defer to a constexpr
>> algorithm, which avoids having to wait for compiler support.
>>
>> Unfortunately I ran out of cycles today to run a full bootstrap/regtest
>> cycle, but constexpr_functions_c++17.cc passes cleanly on
>> x86_64 GNU/Linux at least.
>>
>> If this looks like a reasonable approach, I can maybe try running
>> full tests on the gcc compile farm next week.
>>
>> WDYT?
>
>Reading the patch via the list made me spot something
>that was incorrect.
>
>>
>> -      static char_type*
>> +      static _GLIBCXX17_CONSTEXPR char_type*
>>        assign(char_type* __s, size_t __n, char_type __a)
>>        {
>> +#if __cplusplus > 201402
>> +	if (__constant_string_p(__s)
>
>This should probably have been __constant_char_array_p, but now
>that I look again at the paper, I see I got myself confused -- this
>assign overload is not meant to be constexpr in the first place.
>
>So here's an updated patch that drops that bit.  (Same testing
>as before.)

Pedro, this is OK for trunk now we're in stage 1. Please go ahead and
commit it - thanks.

It's probably safe for gcc-7-branch too, but let's leave it on trunk
for a while first.
diff mbox

Patch

diff --git a/libstdc++-v3/doc/xml/manual/status_cxx2017.xml b/libstdc++-v3/doc/xml/manual/status_cxx2017.xml
index 0e35f75..fed91f9 100644
--- a/libstdc++-v3/doc/xml/manual/status_cxx2017.xml
+++ b/libstdc++-v3/doc/xml/manual/status_cxx2017.xml
@@ -489,8 +489,8 @@  Feature-testing recommendations for C++</link>.
 	P0426R1
 	</link>
       </entry>
-      <entry align="center"> 7 (partial) </entry>
-      <entry><code> ??? </code></entry>
+      <entry align="center"> 7 </entry>
+      <entry><code> __cpp_lib_constexpr_char_traits >= 201611 </code></entry>
     </row>
 
     <row>
diff --git a/libstdc++-v3/include/bits/char_traits.h b/libstdc++-v3/include/bits/char_traits.h
index 75db5b8..3ecc30e 100644
--- a/libstdc++-v3/include/bits/char_traits.h
+++ b/libstdc++-v3/include/bits/char_traits.h
@@ -40,6 +40,10 @@ 
 #include <bits/postypes.h>      // For streampos
 #include <cwchar>               // For WEOF, wmemmove, wmemset, etc.
 
+#ifndef _GLIBCXX_ALWAYS_INLINE
+#define _GLIBCXX_ALWAYS_INLINE inline __attribute__((__always_inline__))
+#endif
+
 namespace __gnu_cxx _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
@@ -139,7 +143,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return !eq_int_type(__c, eof()) ? __c : to_int_type(char_type()); }
     };
 
-// #define __cpp_lib_constexpr_char_traits 201611
+#define __cpp_lib_constexpr_char_traits 201611
 
   template<typename _CharT>
     _GLIBCXX14_CONSTEXPR int
@@ -212,6 +216,42 @@  namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
+#if __cplusplus > 201402
+  /**
+   *  @brief Determine whether the characters of a NULL-terminated
+   *  string are known at compile time.
+   *  @param  __s  The string.
+   *
+   *  Assumes that _CharT is a built-in character type.
+   */
+  template<typename _CharT>
+    static _GLIBCXX_ALWAYS_INLINE constexpr bool
+    __constant_string_p(const _CharT* __s)
+    {
+      while (__builtin_constant_p(*__s) && *__s)
+	__s++;
+      return __builtin_constant_p(*__s);
+    }
+
+  /**
+   *  @brief Determine whether the characters of a character array are
+   *  known at compile time.
+   *  @param  __a  The character array.
+   *  @param  __n  Number of characters.
+   *
+   *  Assumes that _CharT is a built-in character type.
+   */
+  template<typename _CharT>
+    static _GLIBCXX_ALWAYS_INLINE constexpr bool
+    __constant_char_array_p(const _CharT* __a, size_t __n)
+    {
+      size_t __i = 0;
+      while (__builtin_constant_p(__a[__i]) && __i < __n)
+	__i++;
+      return __i == __n;
+    }
+#endif
+
   // 21.1
   /**
    *  @brief  Basis for explicit traits specializations.
@@ -256,21 +296,39 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		< static_cast<unsigned char>(__c2));
       }
 
-      static /* _GLIBCXX17_CONSTEXPR */ int
+      static _GLIBCXX17_CONSTEXPR int
       compare(const char_type* __s1, const char_type* __s2, size_t __n)
       {
+#if __cplusplus > 201402
+	if (__builtin_constant_p(__n)
+	    && __constant_char_array_p(__s1, __n)
+	    && __constant_char_array_p(__s2, __n))
+	  return __gnu_cxx::char_traits<char_type>::compare(__s1, __s2, __n);
+#endif
 	if (__n == 0)
 	  return 0;
 	return __builtin_memcmp(__s1, __s2, __n);
       }
 
-      static /* _GLIBCXX17_CONSTEXPR */ size_t
+      static _GLIBCXX17_CONSTEXPR size_t
       length(const char_type* __s)
-      { return __builtin_strlen(__s); }
+      {
+#if __cplusplus > 201402
+	if (__constant_string_p(__s))
+	  return __gnu_cxx::char_traits<char_type>::length(__s);
+#endif
+	return __builtin_strlen(__s);
+      }
 
-      static /* _GLIBCXX17_CONSTEXPR */ const char_type*
+      static _GLIBCXX17_CONSTEXPR const char_type*
       find(const char_type* __s, size_t __n, const char_type& __a)
       {
+#if __cplusplus > 201402
+	if (__builtin_constant_p(__n)
+	    && __builtin_constant_p(__a)
+	    && __constant_char_array_p(__s, __n))
+	  return __gnu_cxx::char_traits<char_type>::find(__s, __n, __a);
+#endif
 	if (__n == 0)
 	  return 0;
 	return static_cast<const char_type*>(__builtin_memchr(__s, __a, __n));
@@ -347,24 +405,45 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       lt(const char_type& __c1, const char_type& __c2) _GLIBCXX_NOEXCEPT
       { return __c1 < __c2; }
 
-      static /* _GLIBCXX17_CONSTEXPR */ int
+      static _GLIBCXX17_CONSTEXPR int
       compare(const char_type* __s1, const char_type* __s2, size_t __n)
       {
+#if __cplusplus > 201402
+	if (__builtin_constant_p(__n)
+	    && __constant_char_array_p(__s1, __n)
+	    && __constant_char_array_p(__s2, __n))
+	  return __gnu_cxx::char_traits<char_type>::compare(__s1, __s2, __n);
+#endif
 	if (__n == 0)
 	  return 0;
-	return wmemcmp(__s1, __s2, __n);
+	else
+	  return wmemcmp(__s1, __s2, __n);
       }
 
-      static /* _GLIBCXX17_CONSTEXPR */ size_t
+      static _GLIBCXX17_CONSTEXPR size_t
       length(const char_type* __s)
-      { return wcslen(__s); }
+      {
+#if __cplusplus > 201402
+	if (__constant_string_p(__s))
+	  return __gnu_cxx::char_traits<char_type>::length(__s);
+	else
+#endif
+	  return wcslen(__s);
+      }
 
-      static /* _GLIBCXX17_CONSTEXPR */ const char_type*
+      static _GLIBCXX17_CONSTEXPR const char_type*
       find(const char_type* __s, size_t __n, const char_type& __a)
       {
+#if __cplusplus > 201402
+	if (__builtin_constant_p(__n)
+	    && __builtin_constant_p(__a)
+	    && __constant_char_array_p(__s, __n))
+	  return __gnu_cxx::char_traits<char_type>::find(__s, __n, __a);
+#endif
 	if (__n == 0)
 	  return 0;
-	return wmemchr(__s, __a, __n);
+	else
+	  return wmemchr(__s, __a, __n);
       }
 
       static char_type*
diff --git a/libstdc++-v3/testsuite/21_strings/char_traits/requirements/constexpr_functions_c++17.cc b/libstdc++-v3/testsuite/21_strings/char_traits/requirements/constexpr_functions_c++17.cc
index 014caa0..efd280f 100644
--- a/libstdc++-v3/testsuite/21_strings/char_traits/requirements/constexpr_functions_c++17.cc
+++ b/libstdc++-v3/testsuite/21_strings/char_traits/requirements/constexpr_functions_c++17.cc
@@ -74,20 +74,20 @@  template<typename CT>
   }
 
 #ifndef __cpp_lib_constexpr_char_traits
-// #error Feature-test macro for constexpr char_traits is missing
+# error Feature-test macro for constexpr char_traits is missing
 #elif __cpp_lib_constexpr_char_traits != 201611
-// #error Feature-test macro for constexpr char_traits has the wrong value
+# error Feature-test macro for constexpr char_traits has the wrong value
 #endif
 
 static_assert( test_assign<std::char_traits<char>>() );
-// static_assert( test_compare<std::char_traits<char>>() );
-// static_assert( test_length<std::char_traits<char>>() );
-// static_assert( test_find<std::char_traits<char>>() );
+static_assert( test_compare<std::char_traits<char>>() );
+static_assert( test_length<std::char_traits<char>>() );
+static_assert( test_find<std::char_traits<char>>() );
 #ifdef _GLIBCXX_USE_WCHAR_T
 static_assert( test_assign<std::char_traits<wchar_t>>() );
-// static_assert( test_compare<std::char_traits<wchar_t>>() );
-// static_assert( test_length<std::char_traits<wchar_t>>() );
-// static_assert( test_find<std::char_traits<wchar_t>>() );
+static_assert( test_compare<std::char_traits<wchar_t>>() );
+static_assert( test_length<std::char_traits<wchar_t>>() );
+static_assert( test_find<std::char_traits<wchar_t>>() );
 #endif
 static_assert( test_assign<std::char_traits<char16_t>>() );
 static_assert( test_compare<std::char_traits<char16_t>>() );