[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
libcxx-commits at lists.llvm.org
Wed Jun 8 14:48:25 PDT 2022
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.
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();
> 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_))
> 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
> 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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits