public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
@ 2021-05-11 18:27 Antony Polukhin
  2021-05-11 20:00 ` Jonathan Wakely
  0 siblings, 1 reply; 11+ messages in thread
From: Antony Polukhin @ 2021-05-11 18:27 UTC (permalink / raw)
  To: libstdc++, gcc-patches List

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

This patch provides compile time diagnostics for common misuse of
[locale.convenience] functions with std::string as a character type.


2021-05-11  Antony Polukhin  <antoshkka@gmail.com>

PR libstdc++/89728
  * include/bits/locale_facets.h (ctype) Add static assert.
  * testsuite/22_locale/ctype/is/string/89728_neg.cc New test.

-- 
Best regards,
Antony Polukhin

[-- Attachment #2: ctype_compile_time_diag.txt --]
[-- Type: text/plain, Size: 4861 bytes --]

diff --git a/libstdc++-v3/include/bits/locale_facets.h b/libstdc++-v3/include/bits/locale_facets.h
index 03724cf..012857f 100644
--- a/libstdc++-v3/include/bits/locale_facets.h
+++ b/libstdc++-v3/include/bits/locale_facets.h
@@ -136,6 +136,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return __s;
     }
 
+  template<typename>
+    struct __is_string
+    {
+	enum _Value { _value = 0 };
+    };
+
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    struct __is_string<basic_string<_CharT, _Traits, _Alloc> >
+    {
+	enum _Value { _value = 1 };
+    };
 
   // 22.2.1.1  Template class ctype
   // Include host and configuration specific ctype enums for ctype_base.
@@ -614,6 +625,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _CharT>
     class ctype : public __ctype_abstract_base<_CharT>
     {
+#if __cplusplus >= 201103L
+      static_assert(!__is_string<_CharT>::_value,
+		    "std::basic_string used as a character type");
+#endif
     public:
       // Types:
       typedef _CharT			char_type;
diff --git a/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
new file mode 100644
index 0000000..987fa8e
--- /dev/null
+++ b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
@@ -0,0 +1,73 @@
+// { dg-do compile { target c++11 } }
+
+// Copyright (C) 2021 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-error "used as a character type" "" { target *-*-* } 0 }
+
+#include <locale>
+
+template <class Char, int I>
+struct trait: std::char_traits<Char> {};
+
+template <class Char, int I>
+auto make_str()
+{
+  return std::basic_string<Char, trait<Char, I>>{};
+}
+
+void test01()
+{
+  const auto& loc = std::locale::classic();
+
+  std::isspace(std::string{}, loc);		// { dg-error "required from here" }
+  std::isprint(make_str<char, 0>(), loc);	// { dg-error "required from here" }
+  std::iscntrl(make_str<char, 1>(), loc);	// { dg-error "required from here" }
+  std::isupper(make_str<char, 2>(), loc);	// { dg-error "required from here" }
+  std::islower(make_str<char, 3>(), loc);	// { dg-error "required from here" }
+  std::isalpha(make_str<char, 4>(), loc);	// { dg-error "required from here" }
+  std::isdigit(make_str<char, 5>(), loc);	// { dg-error "required from here" }
+  std::ispunct(make_str<char, 6>(), loc);	// { dg-error "required from here" }
+  std::isxdigit(make_str<char, 7>(), loc);	// { dg-error "required from here" }
+  std::isalnum(make_str<char, 8>(), loc);	// { dg-error "required from here" }
+  std::isgraph(make_str<char, 9>(), loc);	// { dg-error "required from here" }
+  std::isblank(make_str<char, 10>(), loc);	// { dg-error "required from here" }
+  std::toupper(make_str<char, 11>(), loc);	// { dg-error "required from here" }
+  std::tolower(make_str<char, 12>(), loc);	// { dg-error "required from here" }
+}
+
+#ifdef _GLIBCXX_USE_WCHAR_T
+void test02()
+{
+  const auto& loc = std::locale::classic();
+
+  std::isspace(std::wstring{}, loc);		// { dg-error "required from here" }
+  std::isprint(make_str<wchar_t, 0>(), loc);	// { dg-error "required from here" }
+  std::iscntrl(make_str<wchar_t, 1>(), loc);	// { dg-error "required from here" }
+  std::isupper(make_str<wchar_t, 2>(), loc);	// { dg-error "required from here" }
+  std::islower(make_str<wchar_t, 3>(), loc);	// { dg-error "required from here" }
+  std::isalpha(make_str<wchar_t, 4>(), loc);	// { dg-error "required from here" }
+  std::isdigit(make_str<wchar_t, 5>(), loc);	// { dg-error "required from here" }
+  std::ispunct(make_str<wchar_t, 6>(), loc);	// { dg-error "required from here" }
+  std::isxdigit(make_str<wchar_t, 7>(), loc);	// { dg-error "required from here" }
+  std::isalnum(make_str<wchar_t, 8>(), loc);	// { dg-error "required from here" }
+  std::isgraph(make_str<wchar_t, 9>(), loc);	// { dg-error "required from here" }
+  std::isblank(make_str<wchar_t, 10>(), loc);	// { dg-error "required from here" }
+  std::toupper(make_str<wchar_t, 11>(), loc);	// { dg-error "required from here" }
+  std::tolower(make_str<wchar_t, 12>(), loc);	// { dg-error "required from here" }
+}
+#endif

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-11 18:27 [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions Antony Polukhin
@ 2021-05-11 20:00 ` Jonathan Wakely
  2021-05-12  9:18   ` Jonathan Wakely
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Wakely @ 2021-05-11 20:00 UTC (permalink / raw)
  To: Antony Polukhin; +Cc: libstdc++, gcc-patches List

On 11/05/21 21:27 +0300, Antony Polukhin via Libstdc++ wrote:
>This patch provides compile time diagnostics for common misuse of
>[locale.convenience] functions with std::string as a character type.
>
>
>2021-05-11  Antony Polukhin  <antoshkka@gmail.com>
>
>PR libstdc++/89728
>  * include/bits/locale_facets.h (ctype) Add static assert.
>  * testsuite/22_locale/ctype/is/string/89728_neg.cc New test.
>
>-- 
>Best regards,
>Antony Polukhin

>diff --git a/libstdc++-v3/include/bits/locale_facets.h b/libstdc++-v3/include/bits/locale_facets.h
>index 03724cf..012857f 100644
>--- a/libstdc++-v3/include/bits/locale_facets.h
>+++ b/libstdc++-v3/include/bits/locale_facets.h
>@@ -136,6 +136,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       return __s;
>     }
>
>+  template<typename>
>+    struct __is_string
>+    {
>+	enum _Value { _value = 0 };

The _value member needs to use double underscores.

But since this is only used in a static_assert, which is only
available in C++11, I think it can just derive from std::false_type
instead.

>+    };
>+
>+  template<typename _CharT, typename _Traits, typename _Alloc>
>+    struct __is_string<basic_string<_CharT, _Traits, _Alloc> >
>+    {
>+	enum _Value { _value = 1 };
>+    };
>
>   // 22.2.1.1  Template class ctype
>   // Include host and configuration specific ctype enums for ctype_base.
>@@ -614,6 +625,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   template<typename _CharT>
>     class ctype : public __ctype_abstract_base<_CharT>
>     {
>+#if __cplusplus >= 201103L
>+      static_assert(!__is_string<_CharT>::_value,
>+		    "std::basic_string used as a character type");
>+#endif
>     public:
>       // Types:
>       typedef _CharT			char_type;

Alternatively, would it be even simpler to just define a partial
specialization of ctype?

template<typename _CharT, typename _Traits, typename _Alloc>
   class ctype<basic_string<_CharT, _Traits, _Alloc> >
   {
#if __cplusplus >= 201103L
       static_assert(something dependent,
		    "std::basic_string used as a character type");
#endif
   private:
     ctype();
     ~ctype();
   };

This will work in C++98 too.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-11 20:00 ` Jonathan Wakely
@ 2021-05-12  9:18   ` Jonathan Wakely
  2021-05-12  9:58     ` Antony Polukhin
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Wakely @ 2021-05-12  9:18 UTC (permalink / raw)
  To: Antony Polukhin; +Cc: libstdc++, gcc-patches List

On 11/05/21 21:00 +0100, Jonathan Wakely wrote:
>Alternatively, would it be even simpler to just define a partial
>specialization of ctype?
>
>template<typename _CharT, typename _Traits, typename _Alloc>
>  class ctype<basic_string<_CharT, _Traits, _Alloc> >
>  {
>#if __cplusplus >= 201103L
>      static_assert(something dependent,
>		    "std::basic_string used as a character type");
>#endif
>  private:
>    ctype();
>    ~ctype();
>  };
>
>This will work in C++98 too.

Or just leave it undefined, as libc++ seems to do according to your
comment in PR 89728:

error: implicit instantiation of undefined template 'std::__1::ctype<std::__1::basic_string<char> >'

Was your aim to have a static_assert that gives a more descriptive
error? We could leave it undefined in C++98 and have the static assert
for C++11 and up.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-12  9:18   ` Jonathan Wakely
@ 2021-05-12  9:58     ` Antony Polukhin
  2021-05-12 12:35       ` Antony Polukhin
  2021-05-12 14:44       ` Jonathan Wakely
  0 siblings, 2 replies; 11+ messages in thread
From: Antony Polukhin @ 2021-05-12  9:58 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches List

ср, 12 мая 2021 г. в 12:18, Jonathan Wakely <jwakely@redhat.com>:
<...>
> Or just leave it undefined, as libc++ seems to do according to your
> comment in PR 89728:
>
> error: implicit instantiation of undefined template 'std::__1::ctype<std::__1::basic_string<char> >'
>
> Was your aim to have a static_assert that gives a more descriptive
> error? We could leave it undefined in C++98 and have the static assert
> for C++11 and up.

Leaving it undefined would be the best. It would allow SFINAE on ctype
and a compile time error is informative enough.

However, there may be users who instantiate ctype<ThierChar> in a
shared library without ctype<ThierChar> template specializations in
the main executable. Making the default ctype undefined would break
their compilation:

#include <locale>
// no ctype<ThierChar> specialization
c = std::tolower(ThierChar{42}, locale_from_shared_library()); // OK
right now in libstdc++, fails on libc++


Should we care about those users?

-- 
Best regards,
Antony Polukhin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-12  9:58     ` Antony Polukhin
@ 2021-05-12 12:35       ` Antony Polukhin
  2021-05-12 14:44       ` Jonathan Wakely
  1 sibling, 0 replies; 11+ messages in thread
From: Antony Polukhin @ 2021-05-12 12:35 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches List

[-- Attachment #1: Type: text/plain, Size: 396 bytes --]

ср, 12 мая 2021 г. в 12:58, Antony Polukhin <antoshkka@gmail.com>:
<...>
> Should we care about those users?

Looks like the answer is "yes". There are tests for that in
22_locale/ctype/requirements/explicit_instantiation.cc and
22_locale/money_get/cons/3.cc

Attaching a patch with review fixes (with ctype specialization for basic_string)

-- 
Best regards,
Antony Polukhin

[-- Attachment #2: ctype_compile_time_diag-3.txt --]
[-- Type: text/plain, Size: 4849 bytes --]

diff --git a/libstdc++-v3/include/bits/locale_facets.h b/libstdc++-v3/include/bits/locale_facets.h
index 03724cf..bbd7da5 100644
--- a/libstdc++-v3/include/bits/locale_facets.h
+++ b/libstdc++-v3/include/bits/locale_facets.h
@@ -671,6 +671,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _CharT>
     locale::id ctype<_CharT>::id;
 
+  // Compile time diagnostics for common misuse of [locale.convenience]
+  // functions with basic_string as a character type.
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    class ctype<basic_string<_CharT, _Traits, _Alloc> >
+    {
+#if __cplusplus >= 201103L
+      static_assert(sizeof(_CharT) && false,
+		    "std::basic_string used as a character type");
+#endif
+      ctype();
+      ~ctype();
+    };
+
   /**
    *  @brief  The ctype<char> specialization.
    *  @ingroup locales
diff --git a/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
new file mode 100644
index 0000000..268e03a
--- /dev/null
+++ b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
@@ -0,0 +1,75 @@
+// { dg-do compile }
+
+// Copyright (C) 2021 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-error "basic_string used as a character type" "" { target c++11 } 0 }
+// { dg-error "is not a member of" "" { target *-*-* } 0 }
+// { dg-error "has no member named" "" { target *-*-* } 0 }
+
+#include <locale>
+
+template <class Char, int I>
+struct trait: std::char_traits<Char> {};
+
+template <class Char, int I>
+std::basic_string<Char, trait<Char, I> > make_str()
+{
+  return std::basic_string<Char, trait<Char, I> >();
+}
+
+void test01()
+{
+  const std::locale& loc = std::locale::classic();
+
+  std::isspace(std::string{}, loc);		// { dg-error "required from here" }
+  std::isprint(make_str<char, 0>(), loc);	// { dg-error "required from here" }
+  std::iscntrl(make_str<char, 1>(), loc);	// { dg-error "required from here" }
+  std::isupper(make_str<char, 2>(), loc);	// { dg-error "required from here" }
+  std::islower(make_str<char, 3>(), loc);	// { dg-error "required from here" }
+  std::isalpha(make_str<char, 4>(), loc);	// { dg-error "required from here" }
+  std::isdigit(make_str<char, 5>(), loc);	// { dg-error "required from here" }
+  std::ispunct(make_str<char, 6>(), loc);	// { dg-error "required from here" }
+  std::isxdigit(make_str<char, 7>(), loc);	// { dg-error "required from here" }
+  std::isalnum(make_str<char, 8>(), loc);	// { dg-error "required from here" }
+  std::isgraph(make_str<char, 9>(), loc);	// { dg-error "required from here" }
+  std::isblank(make_str<char, 10>(), loc);	// { dg-error "required from here" }
+  std::toupper(make_str<char, 11>(), loc);	// { dg-error "required from here" }
+  std::tolower(make_str<char, 12>(), loc);	// { dg-error "required from here" }
+}
+
+#ifdef _GLIBCXX_USE_WCHAR_T
+void test02()
+{
+  const std::locale& loc = std::locale::classic();
+
+  std::isspace(std::wstring{}, loc);		// { dg-error "required from here" }
+  std::isprint(make_str<wchar_t, 0>(), loc);	// { dg-error "required from here" }
+  std::iscntrl(make_str<wchar_t, 1>(), loc);	// { dg-error "required from here" }
+  std::isupper(make_str<wchar_t, 2>(), loc);	// { dg-error "required from here" }
+  std::islower(make_str<wchar_t, 3>(), loc);	// { dg-error "required from here" }
+  std::isalpha(make_str<wchar_t, 4>(), loc);	// { dg-error "required from here" }
+  std::isdigit(make_str<wchar_t, 5>(), loc);	// { dg-error "required from here" }
+  std::ispunct(make_str<wchar_t, 6>(), loc);	// { dg-error "required from here" }
+  std::isxdigit(make_str<wchar_t, 7>(), loc);	// { dg-error "required from here" }
+  std::isalnum(make_str<wchar_t, 8>(), loc);	// { dg-error "required from here" }
+  std::isgraph(make_str<wchar_t, 9>(), loc);	// { dg-error "required from here" }
+  std::isblank(make_str<wchar_t, 10>(), loc);	// { dg-error "required from here" }
+  std::toupper(make_str<wchar_t, 11>(), loc);	// { dg-error "required from here" }
+  std::tolower(make_str<wchar_t, 12>(), loc);	// { dg-error "required from here" }
+}
+#endif

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-12  9:58     ` Antony Polukhin
  2021-05-12 12:35       ` Antony Polukhin
@ 2021-05-12 14:44       ` Jonathan Wakely
  2021-05-12 15:38         ` Antony Polukhin
  1 sibling, 1 reply; 11+ messages in thread
From: Jonathan Wakely @ 2021-05-12 14:44 UTC (permalink / raw)
  To: Antony Polukhin; +Cc: libstdc++, gcc-patches List

On 12/05/21 12:58 +0300, Antony Polukhin wrote:
>ср, 12 мая 2021 г. в 12:18, Jonathan Wakely <jwakely@redhat.com>:
><...>
>> Or just leave it undefined, as libc++ seems to do according to your
>> comment in PR 89728:
>>
>> error: implicit instantiation of undefined template 'std::__1::ctype<std::__1::basic_string<char> >'
>>
>> Was your aim to have a static_assert that gives a more descriptive
>> error? We could leave it undefined in C++98 and have the static assert
>> for C++11 and up.
>
>Leaving it undefined would be the best. It would allow SFINAE on ctype
>and a compile time error is informative enough.
>
>However, there may be users who instantiate ctype<ThierChar> in a
>shared library without ctype<ThierChar> template specializations in
>the main executable. Making the default ctype undefined would break
>their compilation:
>
>#include <locale>
>// no ctype<ThierChar> specialization
>c = std::tolower(ThierChar{42}, locale_from_shared_library()); // OK
>right now in libstdc++, fails on libc++

What I meant was leaving the partial specialization undefined, not the
primary template, i.e.

--- a/libstdc++-v3/include/bits/locale_facets.h
+++ b/libstdc++-v3/include/bits/locale_facets.h
@@ -1476,6 +1476,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
      };
  #endif //_GLIBCXX_USE_WCHAR_T

+  template<typename _CharT, typename _Traits, typename _Alloc>
+    class ctype<basic_string<_CharT, _Traits, _Alloc> >;
+
    /// class ctype_byname [22.2.1.2].
    template<typename _CharT>
      class ctype_byname : public ctype<_CharT>

This makes your test fail with errors like this:

In file included from /home/jwakely/gcc/12/include/c++/12.0.0/locale:40,
                  from loc.C:1:
/home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h: In instantiation of 'bool std::isspace(_CharT, const std::locale&) [with _CharT = std::__cxx11::basic_string<char>]':
loc.C:16:15:   required from here
/home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h:2600:47: error: invalid use of incomplete type 'const class std::ctype<std::__cxx11::basic_string<char> >'
  2600 |     { return use_facet<ctype<_CharT> >(__loc).is(ctype_base::space, __c); }
       |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

But it shouldn't affect the uses of ctype<TheirChar>.

What do you think?



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-12 14:44       ` Jonathan Wakely
@ 2021-05-12 15:38         ` Antony Polukhin
  2021-05-12 15:51           ` Antony Polukhin
  0 siblings, 1 reply; 11+ messages in thread
From: Antony Polukhin @ 2021-05-12 15:38 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches List

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

ср, 12 мая 2021 г. в 17:44, Jonathan Wakely <jwakely@redhat.com>:
>
> On 12/05/21 12:58 +0300, Antony Polukhin wrote:
> >ср, 12 мая 2021 г. в 12:18, Jonathan Wakely <jwakely@redhat.com>:
> ><...>
> >> Or just leave it undefined, as libc++ seems to do according to your
> >> comment in PR 89728:
> >>
> >> error: implicit instantiation of undefined template 'std::__1::ctype<std::__1::basic_string<char> >'
> >>
> >> Was your aim to have a static_assert that gives a more descriptive
> >> error? We could leave it undefined in C++98 and have the static assert
> >> for C++11 and up.
> >
> >Leaving it undefined would be the best. It would allow SFINAE on ctype
> >and a compile time error is informative enough.
> >
> >However, there may be users who instantiate ctype<ThierChar> in a
> >shared library without ctype<ThierChar> template specializations in
> >the main executable. Making the default ctype undefined would break
> >their compilation:
> >
> >#include <locale>
> >// no ctype<ThierChar> specialization
> >c = std::tolower(ThierChar{42}, locale_from_shared_library()); // OK
> >right now in libstdc++, fails on libc++
>
> What I meant was leaving the partial specialization undefined, not the
> primary template, i.e.
>
> --- a/libstdc++-v3/include/bits/locale_facets.h
> +++ b/libstdc++-v3/include/bits/locale_facets.h
> @@ -1476,6 +1476,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       };
>   #endif //_GLIBCXX_USE_WCHAR_T
>
> +  template<typename _CharT, typename _Traits, typename _Alloc>
> +    class ctype<basic_string<_CharT, _Traits, _Alloc> >;
> +
>     /// class ctype_byname [22.2.1.2].
>     template<typename _CharT>
>       class ctype_byname : public ctype<_CharT>
>
> This makes your test fail with errors like this:
>
> In file included from /home/jwakely/gcc/12/include/c++/12.0.0/locale:40,
>                   from loc.C:1:
> /home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h: In instantiation of 'bool std::isspace(_CharT, const std::locale&) [with _CharT = std::__cxx11::basic_string<char>]':
> loc.C:16:15:   required from here
> /home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h:2600:47: error: invalid use of incomplete type 'const class std::ctype<std::__cxx11::basic_string<char> >'
>   2600 |     { return use_facet<ctype<_CharT> >(__loc).is(ctype_base::space, __c); }
>        |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
>
> But it shouldn't affect the uses of ctype<TheirChar>.
>
> What do you think?

Good idea. That way the compiler message points directly to the
misused function.

Patch is in attachment

-- 
Best regards,
Antony Polukhin

[-- Attachment #2: ctype_compile_time_diag-7.txt --]
[-- Type: text/plain, Size: 4540 bytes --]

diff --git a/libstdc++-v3/include/bits/locale_facets.h b/libstdc++-v3/include/bits/locale_facets.h
index 03724cf..5ca431e 100644
--- a/libstdc++-v3/include/bits/locale_facets.h
+++ b/libstdc++-v3/include/bits/locale_facets.h
@@ -671,6 +671,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _CharT>
     locale::id ctype<_CharT>::id;
 
+  // Incomplete to provide a compile time diagnostics for common misuse
+  // of [locale.convenience] functions with basic_string as a character type.
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    class ctype<basic_string<_CharT, _Traits, _Alloc> >;
+
   /**
    *  @brief  The ctype<char> specialization.
    *  @ingroup locales
diff --git a/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
new file mode 100644
index 0000000..ccab3a2
--- /dev/null
+++ b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
@@ -0,0 +1,73 @@
+// { dg-do compile }
+
+// Copyright (C) 2021 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-error "complete" "" { target *-*-* } 0 }
+
+#include <locale>
+
+template <class Char, int I>
+struct trait: std::char_traits<Char> {};
+
+template <class Char, int I>
+std::basic_string<Char, trait<Char, I> > make_str()
+{
+  return std::basic_string<Char, trait<Char, I> >();
+}
+
+void test01()
+{
+  const std::locale& loc = std::locale::classic();
+
+  std::isspace(std::string{}, loc);		// { dg-error "required from here" }
+  std::isprint(make_str<char, 0>(), loc);	// { dg-error "required from here" }
+  std::iscntrl(make_str<char, 1>(), loc);	// { dg-error "required from here" }
+  std::isupper(make_str<char, 2>(), loc);	// { dg-error "required from here" }
+  std::islower(make_str<char, 3>(), loc);	// { dg-error "required from here" }
+  std::isalpha(make_str<char, 4>(), loc);	// { dg-error "required from here" }
+  std::isdigit(make_str<char, 5>(), loc);	// { dg-error "required from here" }
+  std::ispunct(make_str<char, 6>(), loc);	// { dg-error "required from here" }
+  std::isxdigit(make_str<char, 7>(), loc);	// { dg-error "required from here" }
+  std::isalnum(make_str<char, 8>(), loc);	// { dg-error "required from here" }
+  std::isgraph(make_str<char, 9>(), loc);	// { dg-error "required from here" }
+  std::isblank(make_str<char, 10>(), loc);	// { dg-error "required from here" }
+  std::toupper(make_str<char, 11>(), loc);	// { dg-error "required from here" }
+  std::tolower(make_str<char, 12>(), loc);	// { dg-error "required from here" }
+}
+
+#ifdef _GLIBCXX_USE_WCHAR_T
+void test02()
+{
+  const std::locale& loc = std::locale::classic();
+
+  std::isspace(std::wstring{}, loc);		// { dg-error "required from here" }
+  std::isprint(make_str<wchar_t, 0>(), loc);	// { dg-error "required from here" }
+  std::iscntrl(make_str<wchar_t, 1>(), loc);	// { dg-error "required from here" }
+  std::isupper(make_str<wchar_t, 2>(), loc);	// { dg-error "required from here" }
+  std::islower(make_str<wchar_t, 3>(), loc);	// { dg-error "required from here" }
+  std::isalpha(make_str<wchar_t, 4>(), loc);	// { dg-error "required from here" }
+  std::isdigit(make_str<wchar_t, 5>(), loc);	// { dg-error "required from here" }
+  std::ispunct(make_str<wchar_t, 6>(), loc);	// { dg-error "required from here" }
+  std::isxdigit(make_str<wchar_t, 7>(), loc);	// { dg-error "required from here" }
+  std::isalnum(make_str<wchar_t, 8>(), loc);	// { dg-error "required from here" }
+  std::isgraph(make_str<wchar_t, 9>(), loc);	// { dg-error "required from here" }
+  std::isblank(make_str<wchar_t, 10>(), loc);	// { dg-error "required from here" }
+  std::toupper(make_str<wchar_t, 11>(), loc);	// { dg-error "required from here" }
+  std::tolower(make_str<wchar_t, 12>(), loc);	// { dg-error "required from here" }
+}
+#endif

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-12 15:38         ` Antony Polukhin
@ 2021-05-12 15:51           ` Antony Polukhin
  2021-05-12 16:16             ` Jonathan Wakely
  0 siblings, 1 reply; 11+ messages in thread
From: Antony Polukhin @ 2021-05-12 15:51 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches List

[-- Attachment #1: Type: text/plain, Size: 2943 bytes --]

ср, 12 мая 2021 г. в 18:38, Antony Polukhin <antoshkka@gmail.com>:
>
> ср, 12 мая 2021 г. в 17:44, Jonathan Wakely <jwakely@redhat.com>:
> >
> > On 12/05/21 12:58 +0300, Antony Polukhin wrote:
> > >ср, 12 мая 2021 г. в 12:18, Jonathan Wakely <jwakely@redhat.com>:
> > ><...>
> > >> Or just leave it undefined, as libc++ seems to do according to your
> > >> comment in PR 89728:
> > >>
> > >> error: implicit instantiation of undefined template 'std::__1::ctype<std::__1::basic_string<char> >'
> > >>
> > >> Was your aim to have a static_assert that gives a more descriptive
> > >> error? We could leave it undefined in C++98 and have the static assert
> > >> for C++11 and up.
> > >
> > >Leaving it undefined would be the best. It would allow SFINAE on ctype
> > >and a compile time error is informative enough.
> > >
> > >However, there may be users who instantiate ctype<ThierChar> in a
> > >shared library without ctype<ThierChar> template specializations in
> > >the main executable. Making the default ctype undefined would break
> > >their compilation:
> > >
> > >#include <locale>
> > >// no ctype<ThierChar> specialization
> > >c = std::tolower(ThierChar{42}, locale_from_shared_library()); // OK
> > >right now in libstdc++, fails on libc++
> >
> > What I meant was leaving the partial specialization undefined, not the
> > primary template, i.e.
> >
> > --- a/libstdc++-v3/include/bits/locale_facets.h
> > +++ b/libstdc++-v3/include/bits/locale_facets.h
> > @@ -1476,6 +1476,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >       };
> >   #endif //_GLIBCXX_USE_WCHAR_T
> >
> > +  template<typename _CharT, typename _Traits, typename _Alloc>
> > +    class ctype<basic_string<_CharT, _Traits, _Alloc> >;
> > +
> >     /// class ctype_byname [22.2.1.2].
> >     template<typename _CharT>
> >       class ctype_byname : public ctype<_CharT>
> >
> > This makes your test fail with errors like this:
> >
> > In file included from /home/jwakely/gcc/12/include/c++/12.0.0/locale:40,
> >                   from loc.C:1:
> > /home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h: In instantiation of 'bool std::isspace(_CharT, const std::locale&) [with _CharT = std::__cxx11::basic_string<char>]':
> > loc.C:16:15:   required from here
> > /home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h:2600:47: error: invalid use of incomplete type 'const class std::ctype<std::__cxx11::basic_string<char> >'
> >   2600 |     { return use_facet<ctype<_CharT> >(__loc).is(ctype_base::space, __c); }
> >        |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
> >
> > But it shouldn't affect the uses of ctype<TheirChar>.
> >
> > What do you think?
>
> Good idea. That way the compiler message points directly to the
> misused function.
>
> Patch is in attachment

Replaced {} with () in test to be C++98 compatible


-- 
Best regards,
Antony Polukhin

[-- Attachment #2: ctype_compile_time_diag-8.txt --]
[-- Type: text/plain, Size: 4540 bytes --]

diff --git a/libstdc++-v3/include/bits/locale_facets.h b/libstdc++-v3/include/bits/locale_facets.h
index 03724cf..5ca431e 100644
--- a/libstdc++-v3/include/bits/locale_facets.h
+++ b/libstdc++-v3/include/bits/locale_facets.h
@@ -671,6 +671,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _CharT>
     locale::id ctype<_CharT>::id;
 
+  // Incomplete to provide a compile time diagnostics for common misuse
+  // of [locale.convenience] functions with basic_string as a character type.
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    class ctype<basic_string<_CharT, _Traits, _Alloc> >;
+
   /**
    *  @brief  The ctype<char> specialization.
    *  @ingroup locales
diff --git a/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
new file mode 100644
index 0000000..9f15620
--- /dev/null
+++ b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
@@ -0,0 +1,73 @@
+// { dg-do compile }
+
+// Copyright (C) 2021 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-error "complete" "" { target *-*-* } 0 }
+
+#include <locale>
+
+template <class Char, int I>
+struct trait: std::char_traits<Char> {};
+
+template <class Char, int I>
+std::basic_string<Char, trait<Char, I> > make_str()
+{
+  return std::basic_string<Char, trait<Char, I> >();
+}
+
+void test01()
+{
+  const std::locale& loc = std::locale::classic();
+
+  std::isspace(std::string(), loc);		// { dg-error "required from here" }
+  std::isprint(make_str<char, 0>(), loc);	// { dg-error "required from here" }
+  std::iscntrl(make_str<char, 1>(), loc);	// { dg-error "required from here" }
+  std::isupper(make_str<char, 2>(), loc);	// { dg-error "required from here" }
+  std::islower(make_str<char, 3>(), loc);	// { dg-error "required from here" }
+  std::isalpha(make_str<char, 4>(), loc);	// { dg-error "required from here" }
+  std::isdigit(make_str<char, 5>(), loc);	// { dg-error "required from here" }
+  std::ispunct(make_str<char, 6>(), loc);	// { dg-error "required from here" }
+  std::isxdigit(make_str<char, 7>(), loc);	// { dg-error "required from here" }
+  std::isalnum(make_str<char, 8>(), loc);	// { dg-error "required from here" }
+  std::isgraph(make_str<char, 9>(), loc);	// { dg-error "required from here" }
+  std::isblank(make_str<char, 10>(), loc);	// { dg-error "required from here" }
+  std::toupper(make_str<char, 11>(), loc);	// { dg-error "required from here" }
+  std::tolower(make_str<char, 12>(), loc);	// { dg-error "required from here" }
+}
+
+#ifdef _GLIBCXX_USE_WCHAR_T
+void test02()
+{
+  const std::locale& loc = std::locale::classic();
+
+  std::isspace(std::wstring(), loc);		// { dg-error "required from here" }
+  std::isprint(make_str<wchar_t, 0>(), loc);	// { dg-error "required from here" }
+  std::iscntrl(make_str<wchar_t, 1>(), loc);	// { dg-error "required from here" }
+  std::isupper(make_str<wchar_t, 2>(), loc);	// { dg-error "required from here" }
+  std::islower(make_str<wchar_t, 3>(), loc);	// { dg-error "required from here" }
+  std::isalpha(make_str<wchar_t, 4>(), loc);	// { dg-error "required from here" }
+  std::isdigit(make_str<wchar_t, 5>(), loc);	// { dg-error "required from here" }
+  std::ispunct(make_str<wchar_t, 6>(), loc);	// { dg-error "required from here" }
+  std::isxdigit(make_str<wchar_t, 7>(), loc);	// { dg-error "required from here" }
+  std::isalnum(make_str<wchar_t, 8>(), loc);	// { dg-error "required from here" }
+  std::isgraph(make_str<wchar_t, 9>(), loc);	// { dg-error "required from here" }
+  std::isblank(make_str<wchar_t, 10>(), loc);	// { dg-error "required from here" }
+  std::toupper(make_str<wchar_t, 11>(), loc);	// { dg-error "required from here" }
+  std::tolower(make_str<wchar_t, 12>(), loc);	// { dg-error "required from here" }
+}
+#endif

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-12 15:51           ` Antony Polukhin
@ 2021-05-12 16:16             ` Jonathan Wakely
  2021-05-17 17:14               ` Jonathan Wakely
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Wakely @ 2021-05-12 16:16 UTC (permalink / raw)
  To: Antony Polukhin; +Cc: libstdc++, gcc-patches List

On 12/05/21 18:51 +0300, Antony Polukhin via Libstdc++ wrote:
>ср, 12 мая 2021 г. в 18:38, Antony Polukhin <antoshkka@gmail.com>:
>>
>> ср, 12 мая 2021 г. в 17:44, Jonathan Wakely <jwakely@redhat.com>:
>> >
>> > On 12/05/21 12:58 +0300, Antony Polukhin wrote:
>> > >ср, 12 мая 2021 г. в 12:18, Jonathan Wakely <jwakely@redhat.com>:
>> > ><...>
>> > >> Or just leave it undefined, as libc++ seems to do according to your
>> > >> comment in PR 89728:
>> > >>
>> > >> error: implicit instantiation of undefined template 'std::__1::ctype<std::__1::basic_string<char> >'
>> > >>
>> > >> Was your aim to have a static_assert that gives a more descriptive
>> > >> error? We could leave it undefined in C++98 and have the static assert
>> > >> for C++11 and up.
>> > >
>> > >Leaving it undefined would be the best. It would allow SFINAE on ctype
>> > >and a compile time error is informative enough.
>> > >
>> > >However, there may be users who instantiate ctype<ThierChar> in a
>> > >shared library without ctype<ThierChar> template specializations in
>> > >the main executable. Making the default ctype undefined would break
>> > >their compilation:
>> > >
>> > >#include <locale>
>> > >// no ctype<ThierChar> specialization
>> > >c = std::tolower(ThierChar{42}, locale_from_shared_library()); // OK
>> > >right now in libstdc++, fails on libc++
>> >
>> > What I meant was leaving the partial specialization undefined, not the
>> > primary template, i.e.
>> >
>> > --- a/libstdc++-v3/include/bits/locale_facets.h
>> > +++ b/libstdc++-v3/include/bits/locale_facets.h
>> > @@ -1476,6 +1476,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >       };
>> >   #endif //_GLIBCXX_USE_WCHAR_T
>> >
>> > +  template<typename _CharT, typename _Traits, typename _Alloc>
>> > +    class ctype<basic_string<_CharT, _Traits, _Alloc> >;
>> > +
>> >     /// class ctype_byname [22.2.1.2].
>> >     template<typename _CharT>
>> >       class ctype_byname : public ctype<_CharT>
>> >
>> > This makes your test fail with errors like this:
>> >
>> > In file included from /home/jwakely/gcc/12/include/c++/12.0.0/locale:40,
>> >                   from loc.C:1:
>> > /home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h: In instantiation of 'bool std::isspace(_CharT, const std::locale&) [with _CharT = std::__cxx11::basic_string<char>]':
>> > loc.C:16:15:   required from here
>> > /home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h:2600:47: error: invalid use of incomplete type 'const class std::ctype<std::__cxx11::basic_string<char> >'
>> >   2600 |     { return use_facet<ctype<_CharT> >(__loc).is(ctype_base::space, __c); }
>> >        |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
>> >
>> > But it shouldn't affect the uses of ctype<TheirChar>.
>> >
>> > What do you think?
>>
>> Good idea. That way the compiler message points directly to the
>> misused function.
>>
>> Patch is in attachment
>
>Replaced {} with () in test to be C++98 compatible

Looks great, thanks.

I'll test and commit this tomorrow.

(And I haven't forgotten about your other patch, I'm still trying to
convince myself it won't cause problems for code which is technically
UB but "works" today).


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-12 16:16             ` Jonathan Wakely
@ 2021-05-17 17:14               ` Jonathan Wakely
  2021-06-01 18:04                 ` Jonathan Wakely
  0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Wakely @ 2021-05-17 17:14 UTC (permalink / raw)
  To: Antony Polukhin; +Cc: libstdc++, gcc-patches List

On 12/05/21 17:16 +0100, Jonathan Wakely wrote:
>On 12/05/21 18:51 +0300, Antony Polukhin via Libstdc++ wrote:
>>ср, 12 мая 2021 г. в 18:38, Antony Polukhin <antoshkka@gmail.com>:
>>>
>>>ср, 12 мая 2021 г. в 17:44, Jonathan Wakely <jwakely@redhat.com>:
>>>>
>>>> On 12/05/21 12:58 +0300, Antony Polukhin wrote:
>>>> >ср, 12 мая 2021 г. в 12:18, Jonathan Wakely <jwakely@redhat.com>:
>>>> ><...>
>>>> >> Or just leave it undefined, as libc++ seems to do according to your
>>>> >> comment in PR 89728:
>>>> >>
>>>> >> error: implicit instantiation of undefined template 'std::__1::ctype<std::__1::basic_string<char> >'
>>>> >>
>>>> >> Was your aim to have a static_assert that gives a more descriptive
>>>> >> error? We could leave it undefined in C++98 and have the static assert
>>>> >> for C++11 and up.
>>>> >
>>>> >Leaving it undefined would be the best. It would allow SFINAE on ctype
>>>> >and a compile time error is informative enough.
>>>> >
>>>> >However, there may be users who instantiate ctype<ThierChar> in a
>>>> >shared library without ctype<ThierChar> template specializations in
>>>> >the main executable. Making the default ctype undefined would break
>>>> >their compilation:
>>>> >
>>>> >#include <locale>
>>>> >// no ctype<ThierChar> specialization
>>>> >c = std::tolower(ThierChar{42}, locale_from_shared_library()); // OK
>>>> >right now in libstdc++, fails on libc++
>>>>
>>>> What I meant was leaving the partial specialization undefined, not the
>>>> primary template, i.e.
>>>>
>>>> --- a/libstdc++-v3/include/bits/locale_facets.h
>>>> +++ b/libstdc++-v3/include/bits/locale_facets.h
>>>> @@ -1476,6 +1476,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>>       };
>>>>   #endif //_GLIBCXX_USE_WCHAR_T
>>>>
>>>> +  template<typename _CharT, typename _Traits, typename _Alloc>
>>>> +    class ctype<basic_string<_CharT, _Traits, _Alloc> >;
>>>> +
>>>>     /// class ctype_byname [22.2.1.2].
>>>>     template<typename _CharT>
>>>>       class ctype_byname : public ctype<_CharT>
>>>>
>>>> This makes your test fail with errors like this:
>>>>
>>>> In file included from /home/jwakely/gcc/12/include/c++/12.0.0/locale:40,
>>>>                   from loc.C:1:
>>>> /home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h: In instantiation of 'bool std::isspace(_CharT, const std::locale&) [with _CharT = std::__cxx11::basic_string<char>]':
>>>> loc.C:16:15:   required from here
>>>> /home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h:2600:47: error: invalid use of incomplete type 'const class std::ctype<std::__cxx11::basic_string<char> >'
>>>>   2600 |     { return use_facet<ctype<_CharT> >(__loc).is(ctype_base::space, __c); }
>>>>        |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
>>>>
>>>> But it shouldn't affect the uses of ctype<TheirChar>.
>>>>
>>>> What do you think?
>>>
>>>Good idea. That way the compiler message points directly to the
>>>misused function.
>>>
>>>Patch is in attachment
>>
>>Replaced {} with () in test to be C++98 compatible
>
>Looks great, thanks.
>
>I'll test and commit this tomorrow.

Not quite "tomorrow", but it's pushed to trunk now. Thanks again!



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions
  2021-05-17 17:14               ` Jonathan Wakely
@ 2021-06-01 18:04                 ` Jonathan Wakely
  0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Wakely @ 2021-06-01 18:04 UTC (permalink / raw)
  To: Antony Polukhin; +Cc: libstdc++, gcc-patches List

[-- Attachment #1: Type: text/plain, Size: 3324 bytes --]

On 17/05/21 18:14 +0100, Jonathan Wakely wrote:
>On 12/05/21 17:16 +0100, Jonathan Wakely wrote:
>>On 12/05/21 18:51 +0300, Antony Polukhin via Libstdc++ wrote:
>>>ср, 12 мая 2021 г. в 18:38, Antony Polukhin <antoshkka@gmail.com>:
>>>>
>>>>ср, 12 мая 2021 г. в 17:44, Jonathan Wakely <jwakely@redhat.com>:
>>>>>
>>>>>On 12/05/21 12:58 +0300, Antony Polukhin wrote:
>>>>>>ср, 12 мая 2021 г. в 12:18, Jonathan Wakely <jwakely@redhat.com>:
>>>>>><...>
>>>>>>> Or just leave it undefined, as libc++ seems to do according to your
>>>>>>> comment in PR 89728:
>>>>>>>
>>>>>>> error: implicit instantiation of undefined template 'std::__1::ctype<std::__1::basic_string<char> >'
>>>>>>>
>>>>>>> Was your aim to have a static_assert that gives a more descriptive
>>>>>>> error? We could leave it undefined in C++98 and have the static assert
>>>>>>> for C++11 and up.
>>>>>>
>>>>>>Leaving it undefined would be the best. It would allow SFINAE on ctype
>>>>>>and a compile time error is informative enough.
>>>>>>
>>>>>>However, there may be users who instantiate ctype<ThierChar> in a
>>>>>>shared library without ctype<ThierChar> template specializations in
>>>>>>the main executable. Making the default ctype undefined would break
>>>>>>their compilation:
>>>>>>
>>>>>>#include <locale>
>>>>>>// no ctype<ThierChar> specialization
>>>>>>c = std::tolower(ThierChar{42}, locale_from_shared_library()); // OK
>>>>>>right now in libstdc++, fails on libc++
>>>>>
>>>>>What I meant was leaving the partial specialization undefined, not the
>>>>>primary template, i.e.
>>>>>
>>>>>--- a/libstdc++-v3/include/bits/locale_facets.h
>>>>>+++ b/libstdc++-v3/include/bits/locale_facets.h
>>>>>@@ -1476,6 +1476,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>>>>      };
>>>>>  #endif //_GLIBCXX_USE_WCHAR_T
>>>>>
>>>>>+  template<typename _CharT, typename _Traits, typename _Alloc>
>>>>>+    class ctype<basic_string<_CharT, _Traits, _Alloc> >;
>>>>>+
>>>>>    /// class ctype_byname [22.2.1.2].
>>>>>    template<typename _CharT>
>>>>>      class ctype_byname : public ctype<_CharT>
>>>>>
>>>>>This makes your test fail with errors like this:
>>>>>
>>>>>In file included from /home/jwakely/gcc/12/include/c++/12.0.0/locale:40,
>>>>>                  from loc.C:1:
>>>>>/home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h: In instantiation of 'bool std::isspace(_CharT, const std::locale&) [with _CharT = std::__cxx11::basic_string<char>]':
>>>>>loc.C:16:15:   required from here
>>>>>/home/jwakely/gcc/12/include/c++/12.0.0/bits/locale_facets.h:2600:47: error: invalid use of incomplete type 'const class std::ctype<std::__cxx11::basic_string<char> >'
>>>>>  2600 |     { return use_facet<ctype<_CharT> >(__loc).is(ctype_base::space, __c); }
>>>>>       |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
>>>>>
>>>>>But it shouldn't affect the uses of ctype<TheirChar>.
>>>>>
>>>>>What do you think?
>>>>
>>>>Good idea. That way the compiler message points directly to the
>>>>misused function.
>>>>
>>>>Patch is in attachment
>>>
>>>Replaced {} with () in test to be C++98 compatible
>>
>>Looks great, thanks.
>>
>>I'll test and commit this tomorrow.
>
>Not quite "tomorrow", but it's pushed to trunk now. Thanks again!

I've also pushed this fix for the new test, so it passes with
-std=gnu++98.

Tested x86_64-linux.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 2096 bytes --]

commit b514fce354b5309a9c232a3fe9347e3abde4385f
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jun 1 19:01:37 2021

    libstdc++: Fix new test for C++98 mode [PR 89728]
    
    The isblank class is not supported until C++11.
    
    Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
    
    libstdc++-v3/ChangeLog:
    
            * testsuite/22_locale/ctype/is/string/89728_neg.cc: Only test
            isblank for C++11 and later.

diff --git a/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
index 9f15620c9a8..89843b68494 100644
--- a/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
+++ b/libstdc++-v3/testsuite/22_locale/ctype/is/string/89728_neg.cc
@@ -45,7 +45,9 @@ void test01()
   std::isxdigit(make_str<char, 7>(), loc);	// { dg-error "required from here" }
   std::isalnum(make_str<char, 8>(), loc);	// { dg-error "required from here" }
   std::isgraph(make_str<char, 9>(), loc);	// { dg-error "required from here" }
-  std::isblank(make_str<char, 10>(), loc);	// { dg-error "required from here" }
+#if __cplusplus >= 201103
+  std::isblank(make_str<char, 10>(), loc);	// { dg-error "required from here" "" { target c++11 } }
+#endif
   std::toupper(make_str<char, 11>(), loc);	// { dg-error "required from here" }
   std::tolower(make_str<char, 12>(), loc);	// { dg-error "required from here" }
 }
@@ -66,7 +68,9 @@ void test02()
   std::isxdigit(make_str<wchar_t, 7>(), loc);	// { dg-error "required from here" }
   std::isalnum(make_str<wchar_t, 8>(), loc);	// { dg-error "required from here" }
   std::isgraph(make_str<wchar_t, 9>(), loc);	// { dg-error "required from here" }
-  std::isblank(make_str<wchar_t, 10>(), loc);	// { dg-error "required from here" }
+#if __cplusplus >= 201103
+  std::isblank(make_str<wchar_t, 10>(), loc);	// { dg-error "required from here" "" { target c++11 } }
+#endif
   std::toupper(make_str<wchar_t, 11>(), loc);	// { dg-error "required from here" }
   std::tolower(make_str<wchar_t, 12>(), loc);	// { dg-error "required from here" }
 }

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-06-01 18:04 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-11 18:27 [PATCH] PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions Antony Polukhin
2021-05-11 20:00 ` Jonathan Wakely
2021-05-12  9:18   ` Jonathan Wakely
2021-05-12  9:58     ` Antony Polukhin
2021-05-12 12:35       ` Antony Polukhin
2021-05-12 14:44       ` Jonathan Wakely
2021-05-12 15:38         ` Antony Polukhin
2021-05-12 15:51           ` Antony Polukhin
2021-05-12 16:16             ` Jonathan Wakely
2021-05-17 17:14               ` Jonathan Wakely
2021-06-01 18:04                 ` Jonathan Wakely

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).