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

[Jonathan Wakely <jwakely@redhat.com>]

[-- 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" }
 }