[libc++] Fix incomplete user-defined ctype specialization in test#74630
[libc++] Fix incomplete user-defined ctype specialization in test#74630
Conversation
The specialization was non-conforming because it was missing a bunch of member functions. Those were missing probably just as an oversight coupled with a bit of laziness -- the rule that user-defined specializations need to match the base template is usually OK to take with a grain of salt, but not when the code is supposed to be portable, which our test suite aims to be. Fixes llvm#74214
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesThe specialization was non-conforming because it was missing a bunch of member functions. Those were missing probably just as an oversight coupled with a bit of laziness -- the rule that user-defined specializations need to match the base template is usually OK to take with a grain of salt, but not when the code is supposed to be portable, which our test suite aims to be. Fixes #74214 Full diff: https://github.com/llvm/llvm-project/pull/74630.diff 1 Files Affected:
diff --git a/libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/user_defined_char_type.pass.cpp b/libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/user_defined_char_type.pass.cpp
index d7b4b816d975b..9a4a2f0d5657e 100644
--- a/libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/user_defined_char_type.pass.cpp
+++ b/libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/user_defined_char_type.pass.cpp
@@ -16,8 +16,6 @@
#include <locale>
#include <string>
-#include "test_macros.h"
-
struct Char {
Char() = default;
Char(char c) : underlying_(c) {}
@@ -73,15 +71,53 @@ struct char_traits<Char> {
static int_type eof() { return char_traits<char>::eof(); }
};
+// This ctype specialization treats all characters as spaces
template <>
-class ctype<Char> : public locale::facet {
+class ctype<Char> : public locale::facet, public ctype_base {
public:
+ using char_type = Char;
static locale::id id;
- Char toupper(Char c) const { return Char(std::toupper(c.underlying_)); }
- const char* widen(const char* first, const char* last, Char* dst) const {
- for (; first != last;)
- *dst++ = Char(*first++);
- return last;
+ explicit ctype(std::size_t refs = 0) : locale::facet(refs) {}
+
+ bool is(mask m, char_type) const { return m & ctype_base::space; }
+ const char_type* is(const char_type* low, const char_type* high, mask* vec) const {
+ for (; low != high; ++low)
+ *vec++ = ctype_base::space;
+ return high;
+ }
+
+ const char_type* scan_is(mask m, const char_type* beg, const char_type* end) const {
+ for (; beg != end; ++beg)
+ if (this->is(m, *beg))
+ return beg;
+ return end;
+ }
+
+ const char_type* scan_not(mask m, const char_type* beg, const char_type* end) const {
+ for (; beg != end; ++beg)
+ if (!this->is(m, *beg))
+ return beg;
+ return end;
+ }
+
+ char_type toupper(char_type c) const { return c; }
+ const char_type* toupper(char_type*, const char_type* end) const { return end; }
+
+ char_type tolower(char_type c) const { return c; }
+ const char_type* tolower(char_type*, const char_type* end) const { return end; }
+
+ char_type widen(char c) const { return char_type(c); }
+ const char* widen(const char* beg, const char* end, char_type* dst) const {
+ for (; beg != end; ++beg, ++dst)
+ *dst = char_type(*beg);
+ return end;
+ }
+
+ char narrow(char_type c, char /*dflt*/) const { return c.underlying_; }
+ const char_type* narrow(const char_type* beg, const char_type* end, char /*dflt*/, char* dst) const {
+ for (; beg != end; ++beg, ++dst)
+ *dst = beg->underlying_;
+ return end;
}
};
|
| // This ctype specialization treats all characters as spaces | ||
| template <> | ||
| class ctype<Char> : public locale::facet { | ||
| class ctype<Char> : public locale::facet, public ctype_base { |
There was a problem hiding this comment.
I'm getting errors here:
D:\GitHub\STL\llvm-project\libcxx\test\std\localization\locale.categories\category.numeric\locale.num.get\user_defined_char_type.pass.cpp(76,21): error: direct base 'locale::facet' is inaccessible due to ambiguity:
class std::ctype<Char> -> class locale::facet
class std::ctype<Char> -> ctype_base -> class locale::facet [-Werror,-Winaccessible-base]
class ctype<Char> : public locale::facet, public ctype_base {
^~~~~~~~~~~~~~~~~~~~
D:\GitHub\STL\out\x64\out\inc\xlocale(454,12): error: ambiguous cast from base 'std::locale::facet' to derived 'std::ctype<Char>':
class locale::facet -> const class std::ctype<Char>
class locale::facet -> ctype_base -> const class std::ctype<Char>
return static_cast<const _Facet&>(*_Pf); // should be dynamic_cast
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I see that your PR is imitating [locale.ctype.general] which depicts template<class charT> class ctype : public locale::facet, public ctype_base. Unfortunately, microsoft/STL is weird - we have ctype_base deriving from locale::facet (which we shouldn't, but fixing it would break ABI), then our ctype derives from only ctype_base.
I'm not sure how to work around this divergence without adding an ifdef for MSVC's STL to this test. I'd be okay with you merging this PR as-is, then I'll look into what else is needed to get it to pass for us.
There was a problem hiding this comment.
Yeah I noticed that you folks had locale::facet deriving from ctype_base (or the other way around) when I tested this on Godbolt. I think it makes it impossible for users to implement a specialization of ctype in a conforming way, but I understand this is not really something you can fix.
I'd be OK with an #ifdef for this case.
|
So I'm going to merge this and then @StephanTLavavej can fix it up for MSVC. |
|
Thank you! 😻 |
The specialization was non-conforming because it was missing a bunch of member functions. Those were missing probably just as an oversight coupled with a bit of laziness -- the rule that user-defined specializations need to match the base template is usually OK to take with a grain of salt, but not when the code is supposed to be portable, which our test suite aims to be.
Fixes #74214