[libcxx-commits] [PATCH] D150015: [libc++] Consistently enable __CORRECT_ISO_CPP_WCHAR_H_PROTO in mbstate.

[Jordan Rupprecht via Phabricator via libcxx-commits]

rupprecht added a comment.

In D150015#4324668 <https://reviews.llvm.org/D150015#4324668>, @philnik wrote:

> IMO we should just `#include <wchar.h>` and `<uchar.h>` instead. That way we don't have to do anything funky in multiple headers. In fact, our `wchar.h` fixes up some interfaces for non-C++-friendly C libraries.

The libc++ `wchar.h` file includes `__mbstate_t.h` though:

  // In libcxx/include/wchar.h:
  #  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

Wouldn't that cause a circular reference between mbstate and wchar? Sorry, this kind of layering between different headers is beyond my expertise.
That said, I did try it, and this works for me:

  #elif !defined(_LIBCPP_HAS_NO_WIDE_CHARACTERS) && __has_include(<wchar.h>)
  #   include <wchar.h> // fall back to the C standard provider of mbstate_t

Just not sure if that is supposed to work generally, and it's a less trivial change than I was planning to make. If that's what you meant, I can update the patch.

In D150015#4324659 <https://reviews.llvm.org/D150015#4324659>, @MaskRay wrote:

> The below demonstrate the glibc<2.26 breakage caused by D148542 <https://reviews.llvm.org/D148542>.
>
> glibc before 2.26 did not provide [`bits/types/mbstate_t.h`](https://sourceware.org/git/?p=glibc.git;a=commit;h=199fc19d3aaaf57944ef036e15904febe877fc93).
> If we include `iosfwd` before `cwchar`, we will include glibc `wchar.h` without defining `__CORRECT_ISO_CPP_WCHAR_H_PROTO` and will get the "incorrect" declarations.
>
>   #ifdef X
>   #include <iosfwd>
>   #include <cwchar>
>   #else
>   #include <cwchar>
>   #include <iosfwd>
>   #endif
>   int main() {
>     wchar_t *v1;
>     const wchar_t *cv2 = L"/";
>     v1 = wcsstr(cv2, L"/");  // should fail
>   }

Thanks, that repro explains the problem this patch is fixing and works outside of a modules build.



================
Comment at: libcxx/include/__mbstate_t.h:33
+#ifdef __cplusplus
+#  define __CORRECT_ISO_CPP_WCHAR_H_PROTO
+#endif
----------------
Mordante wrote:
> MaskRay wrote:
> > Mordante wrote:
> > > This looks wrong. When users include both `wchar.h` and `uchar.h` the system may try to define `__CORRECT_ISO_CPP_WCHAR_H_PROTO` twice.
> > This is fine. For
> > ```
> > #  define __CORRECT_ISO_CPP_WCHAR_H_PROTO
> > #  define __CORRECT_ISO_CPP_WCHAR_H_PROTO
> > ```
> > 
> > The two replacement lists (empty) are identical and are allowed. Actually, this is what will happen with a glibc newer than 2019-12 (https://sourceware.org/bugzilla/show_bug.cgi?id=25232). glibc `wchar.h` defines the macro `__CORRECT_ISO_CPP_WCHAR_H_PROTO` as well.
> Ah yes good point.
If preferred, I suppose we could write it as this:

```
#ifdef __cplusplus && !defined(__CORRECT_ISO_CPP_WCHAR_H_PROTO)
#  define __CORRECT_ISO_CPP_WCHAR_H_PROTO
#endf
```

But I agree with Ray that it should be fine to leave it as-is; the duplicate identical definitions shouldn't conflict.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150015/new/

https://reviews.llvm.org/D150015