[libc++] cuchar redeclares ::mbstate_t when it's in its own clang module

[iana]

Summary

When cuchar is compiled independently on platforms that don't have <uchar.h>, it doesn't get a declaration of mbstate_t, and so makes an empty declaration for ::mbstate_t. That conflicts with the declarations in __mbstate_t.h and cwchar since both of those headers do get mbstate_t before making ::mbstate_t.

Change __mbstate_t.h to just get the underlying declaration for mbstate_t and not make a declaration for ::mbstate_t. Include __mbstate_t.h in uchar.h and wchar.h when their next headers aren't available so that at least mbstate_t gets defined.

Add __std_mbstate_t.h to declare ::mbstate_t for headers that need that when cuchar or cwchar aren't available (because either _LIBCPP_HAS_NO_WIDE_CHARACTERS or _LIBCPP_CXX03_LANG).

[philnik]

Why do we want to change this? The current version looks just fine.

[iana]

In D148542#4274570, @philnik wrote:

Why do we want to change this? The current version looks just fine.

The redeclaration becomes an error with clang modules when __mbstate.h and cuchar are in different modules.

[Mordante]

LGTM modulo one comment. I leave the final approval to @philnik.

libcxx/include/cuchar

53 ↗

(On Diff #514279)

Can you update this comment why this is needed, this is useful for future readers of the code.

[philnik]

In D148542#4274624, @iana wrote:

In D148542#4274570, @philnik wrote:

Why do we want to change this? The current version looks just fine.

The redeclaration becomes an error with clang modules when __mbstate.h and cuchar are in different modules.

Please add that information to the commit message.

LGTM with comments addressed.

libcxx/include/cuchar

52 ↗

(On Diff #514279)

I don't think this check makes sense. It seems like right now we might have this symbol in C++03 in some headers, but not in cuchar. If we only want this in C++11 we should guard it in __mbstate_t.h.

[iana]

In D148542#4274624, @iana wrote:

In D148542#4274570, @philnik wrote:

Why do we want to change this? The current version looks just fine.

The redeclaration becomes an error with clang modules when __mbstate_t.h and cuchar are in different modules.

I dug into it a little more. When <cuchar> is compiled all by itself on Apple platforms, mbstate_t isn't defined because Apple doesn't have a <uchar.h>, and <stddef.h> doesn't declare mbstate_t either. But when <__mbstate_t.h> (and <cwchar>) are compiled, they both get the OS declaration of mbstate_t and their using declaration gets a type. I think it works out with the current std mega module because declarations leak between the submodules, but when those three headers are in different top level modules, that's when things get mad at <cuchar>.

So it looks like it's fine to leave the using ::mbstate_t _LIBCPP_USING_IF_EXISTS; line in <cuchar>, but we need to include something to get mbstate_t defined. Do we want to unconditionally include __mbstate_t.h? That will have the side effect of often including <wchar.h>, and also declaring ::mbstate_t which maybe we don't want. We could leave that include conditional on _LIBCPP_CXX03_LANG? Or we could instead include <bits/types/mbstate_t.h> or <sys/_types/_mbstate_t.h> to get just the C declaration?

[iana]

Redo the fix to make the libc++ uchar.h try harder to find mbstate_t.

[iana]

The sanitizer and aix test failures don't look related to this change, I get the same ones in an unrelated review too.

libcxx/include/uchar.h

51

This should maybe be

# define __need_size_t
# include_next <stddef.h>

Or maybe that's a bit too stringent.

[ldionne]

libcxx/include/uchar.h

51

Shouldn't we include __mbstate_t.h instead? Then we can tweak what that header does as needed to avoid including wchar.h from it unconditionally, perhaps?

[iana]

libcxx/include/uchar.h

51

The problem with including __mbstate_t.h here is it adds ::mbstate_t which I don't think we want in the C header. I was originally doing that in cuchar, but then that was kind of weird because it would declare ::mbstate_t all the time when the rest of the file is gated on _LIBCPP_CXX03_LANG. People thought it was weird to conditionally include __mbstate_t.h in cuchar too. Also __mbstate_t.h starts off trying to include all of wchar.h which seems counter to the comment at its top?

[iana]

Edits with feedback from @ldionne

[ldionne]

So hard for such a small patch! Thanks for working through this with me!

libcxx/include/__mbstate_t.h

18–25

34–38

libcxx/include/wchar.h

126

[iana]

Review changes, fix failed test.

[iana]

__locale uses mbstate_t without including __mbstate_t.h

[iana]

std::mbstate_t is still needed even when uchar/wchar aren't available. Make a __std_mbstate_t.h header for that.

[iana]

Revert ignore_format.txt

[ldionne]

This LGTM to merge, the CI failures are unrelated, I just checked.

[sylvestre.ledru]

i wonder if this isn't caused this regression:
https://github.com/llvm/llvm-project/issues/62523

[sylvestre.ledru]

I confirmed that a local revert fixed the issue for me

libcxx/include/__mbstate_t.h

 #ifndef _LIBCPP___MBSTATE_T_H
 #define _LIBCPP___MBSTATE_T_H
 
 #include <__config>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
-// TODO(ldionne):
-// The goal of this header is to provide mbstate_t without having to pull in
-// <wchar.h> or <uchar.h>. This is necessary because we need that type even
-// when we don't have (or try to provide) support for wchar_t, because several
-// types like std::fpos are defined in terms of mbstate_t.
+// The goal of this header is to provide mbstate_t without requiring all of
+// <uchar.h> or <wchar.h>. It's also used by the libc++ versions of <uchar.h>
+// and <wchar.h> to get mbstate_t when the C library doesn't provide <uchar.h>
+// or <wchar.h>, hence the #include_next of those headers instead of #include.
+// (e.g. if <wchar.h> isn't present in the C library, the libc++ <wchar.h>
+// will include this header. This header needs to not turn around and cyclically
+// include <wchar.h>, but fall through to <uchar.h>.)
 //
-// This is a gruesome hack, but I don't know how to make it cleaner for
-// the time being.
+// This does not define std::mbstate_t -- this only brings in the declaration
+// in the global namespace.
 
-#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
-#   include <wchar.h> // for mbstate_t
-#elif __has_include(<bits/types/mbstate_t.h>)
+#if __has_include(<bits/types/mbstate_t.h>)
 #   include <bits/types/mbstate_t.h> // works on most Unixes
 #elif __has_include(<sys/_types/_mbstate_t.h>)
 #   include <sys/_types/_mbstate_t.h> // works on Darwin
+#elif !defined(_LIBCPP_HAS_NO_WIDE_CHARACTERS) && __has_include_next(<wchar.h>)
+#   include_next <wchar.h> // fall back to the C standard provider of mbstate_t
+#elif __has_include_next(<uchar.h>)
+#   include_next <uchar.h> // <uchar.h> is also required to make mbstate_t visible
 #else
-#   error "The library was configured without support for wide-characters, but we don't know how to get the definition of mbstate_t without <wchar.h> on your platform."
+#   error "We don't know how to get the definition of mbstate_t without <wchar.h> on your platform."
 #endif
 
-_LIBCPP_BEGIN_NAMESPACE_STD
-
-using ::mbstate_t _LIBCPP_USING_IF_EXISTS;
-
-_LIBCPP_END_NAMESPACE_STD
-
 #endif // _LIBCPP___MBSTATE_T_H

libcxx/include/uchar.h

 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #if !defined(_LIBCPP_CXX03_LANG)
 
 // Some platforms don't implement <uchar.h> and we don't want to give a hard
 // error on those platforms. When the platform doesn't provide <uchar.h>, at
-// least include <stddef.h> so we get the declaration for size_t.
+// least include <stddef.h> so we get the declaration for size_t, and try to
+// get the declaration of mbstate_t too.
 #if __has_include_next(<uchar.h>)
 # include_next <uchar.h>
 #else
+# include <__mbstate_t.h>
 # include <stddef.h>
 #endif
 
 #endif // _LIBCPP_CXX03_LANG
 
 #endif // _LIBCPP_UCHAR_H

libcxx/include/wchar.h

 #endif
 
 #ifdef __cplusplus
 #define __CORRECT_ISO_CPP_WCHAR_H_PROTO
 #endif
 
 #  if __has_include_next(<wchar.h>)
 #    include_next <wchar.h>
+#  else
+#    include <__mbstate_t.h> // make sure we have mbstate_t regardless of the existence of <wchar.h>
 #  endif
 
 // Determine whether we have const-correct overloads for wcschr and friends.
 #if defined(_WCHAR_H_CPLUSPLUS_98_CONFORMANCE_)
 #  define _LIBCPP_WCHAR_H_HAS_CONST_OVERLOADS 1
 #elif defined(__GLIBC_PREREQ)
 #  if __GLIBC_PREREQ(2, 10)
 #    define _LIBCPP_WCHAR_H_HAS_CONST_OVERLOADS 1

libcxx/test/std/strings/string.view/string.view.comparison/comparison.verify.cpp

 
 // template<class charT, class traits>
 //   constexpr auto operator<=>(basic_string_view<charT, traits> lhs, basic_string_view<charT, traits> rhs);
 //
 // LWG 3432
 // [string.view]/4
 // Mandates: R denotes a comparison category type ([cmp.categories]).
 
+#include <cwchar>
 #include <string_view>
 
 #include "test_macros.h"
 
 template <class CharT, class Ordering>
 struct traits {
   typedef CharT char_type;
   typedef int int_type;