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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In its current state, this patch would cause any new target where ` wint_t` can't represent all values of `CharType` to have the same bug. IIUC, the same is true on existing targets like x86_64 Darwin with a custom `CharType` that has the same size as `int_type` -- is that correct? If so, I think we should instead fix the problem by default, but provide an escape hatch for existing targets where this would break the ABI to opt out. For example:

  template <class _Traits>
  struct _OptionalFill {
    _OptionalFill() : __set_(false) { }
    _OptionalFill& operator=(typename _Traits::int_type __x) { __set_ = true; __fill_ = __x; return *this; }
    bool __is_set() const { return __set_; }
    typename _Traits::int_type __fill() const { return __fill_; }
  
  private:
    typename _Traits::int_type __fill_;
    bool __set_;
  };
  
  template <class _Traits>
  struct _SentinelValueFill {
    _SentinelValueFill() : __fill_(_Traits::eof()) { }
    _SentinelValueFill& operator=(typename _Traits::int_type __x) { __fill_ = __x; return *this; }
    bool __is_set() const { return __fill_ == _Traits::eof(); }
    typename _Traits::int_type __fill() const { return __fill_; }
  
  private:
    typename _Traits::int_type __fill_;
  };
  
  template <class _CharT, class _Traits>
  class basic_ios {
    // ...
  
  private:
    basic_ostream<char_type, traits_type>* __tie_;
  
  #if defined(_WIN32) || defined(whatever)
    static constexpr bool _OptOutForABICompat = true;
  #else
    static constexpr bool _OptOutForABICompat = false;
  #endif
  
    // Here, basically always use an appropriate representation for the fill, except on
    // platforms where we'd rather keep the bug in place than break the ABI.
    using _FillType = _If<sizeof(char_type) >= sizeof(typename traits_type::int_type) && !_OptOutForABICompat, _OptionalFill<_Traits>, _SentinelValueFill<_Traits> >;
    _FillType __fill_;
  };

Thoughts?

Also, please make sure to follow https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line when uploading patches, it ensures that there's context in Phabricator, which is useful for reviewing :-)



================
Comment at: libcxx/include/ios:707
     __tie_ = nullptr;
     __fill_ = traits_type::eof();
+#if defined(_LIBCPP_BASIC_IOS_HAS_FILL_SET)
----------------
Do we need to set the `__fill_` when we use `__fill_set_`?


================
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?


================
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?


================
Comment at: libcxx/test/std/input.output/iostream.format/std.manip/setfill_wcharmax.pass.cpp:17
+
+// template<charT> T4 setfill(charT c);
+
----------------



================
Comment at: libcxx/test/std/input.output/iostream.format/std.manip/setfill_wcharmax.pass.cpp:35
+{
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+    {
----------------
Let's use `// UNSUPPORTED: no-wide-characters` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124555