diff mbox series

PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions

Message ID CAKqmYPYf-Cp3Q68DuJwUzLNGZwS5b=tuzYoZk=dQLp=wFc5ESg@mail.gmail.com
State New
Headers show
Series PR libstdc++/89728 diagnose some missuses of [locale.convenience] functions | expand

Commit Message

Antony Polukhin May 11, 2021, 6:27 p.m. UTC
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.

Comments

Jonathan Wakely May 11, 2021, 8 p.m. UTC | #1
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.
Jonathan Wakely May 12, 2021, 9:18 a.m. UTC | #2
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.
Antony Polukhin May 12, 2021, 9:58 a.m. UTC | #3
ср, 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?
Antony Polukhin May 12, 2021, 12:35 p.m. UTC | #4
ср, 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)
Jonathan Wakely May 12, 2021, 2:44 p.m. UTC | #5
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?
Antony Polukhin May 12, 2021, 3:38 p.m. UTC | #6
ср, 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
Antony Polukhin May 12, 2021, 3:51 p.m. UTC | #7
ср, 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
Jonathan Wakely May 12, 2021, 4:16 p.m. UTC | #8
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).
Jonathan Wakely May 17, 2021, 5:14 p.m. UTC | #9
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!
Jonathan Wakely June 1, 2021, 6:04 p.m. UTC | #10
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.
diff mbox series

Patch

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