[libcxx-commits] [PATCH] D124555: [libcxx][AIX][z/OS] basic_ios<wchar_t> cannot store fill character WCHAR_MAX

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm added a comment.

  using _FillType = _If<sizeof(char_type) >= sizeof(typename traits_type::int_type) && !_OptOutForABICompat, _OptionalFill<_Traits>, _SentinelValueFill<_Traits> >;

The problem isn't necessarily exposed based on the type size.  For example: if wchar is 4 bytes signed then WCHAR_MAX is 0x7fffffff and WEOF is 0xffffffff so the sentinel value can be represented.

I suggest:

  using _FillType = _If<traits_type::eq_int_type(std::numeric_limits<char_type>::max(), traits_type::eof) && !_OptOutForABICompat, _OptionalFill<_Traits>, _SentinelValueFill<_Traits> >;



================
Comment at: libcxx/include/ios:707
     __tie_ = nullptr;
     __fill_ = traits_type::eof();
+#if defined(_LIBCPP_BASIC_IOS_HAS_FILL_SET)
----------------
ldionne wrote:
> Do we need to set the `__fill_` when we use `__fill_set_`?
I agree, we don't have to initialize `__fill_` if we have `__fill_set_`.  I was trying to mitigate the need to check `__fill_set_` elsewhere but I think we will always need to on the first call.


================
Comment at: libcxx/include/ios:782
 {
     if (traits_type::eq_int_type(traits_type::eof(), __fill_))
+    {
----------------
ldionne wrote:
> When we use `__fill_set_`, isn't this condition redundant?
I wrote it this way to mitigate the need to check the `__fill_set_` member to minimize the impact of the ABI breakage.  The only case you really need to check `__fill_set` is if the fill charafter was is eof.

Reflecting now I think the effort was misguided because you're always going to get in here on the first call to `fill`.  Probably better to keep it simple than get cute.


================
Comment at: libcxx/test/std/input.output/iostream.format/std.manip/setfill_wcharmax.pass.cpp:9-12
+// XFAIL: stdlib=libc++ && target=aarch64-linux-gnu
+// XFAIL: stdlib=libc++ && target=armv7l-linux-gnueabihf
+// XFAIL: stdlib=libc++ && target=armv8l-linux-gnueabihf
+// XFAIL: stdlib=libc++ && target=x86_64-pc-windows-msvc
----------------
ldionne wrote:
> Do I understand correctly that all of those targets currently have a bug that one can't set a fill char to `WCHAR_MAX`, however fixing that requires breaking the ABI?
Those were all the targets the failed the new test via the CI when I submitted the first version of the patch.  I believe these targets will need to break the ABI if they want the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124555