[libc++] Fix initialization of __fill_

[Jake-Egan]

Summary

basic_ios delays initialization of __fill_ to widen(' ') until fill() is called. But, fill(char_type) is missing this logic, so the fill character does not get initialized to whitespace if fill(char_type) is called first. This patch adds this logic to fill(char_type).

[Jake-Egan]

Fix indentation.

[• Quuxplusone]

libcxx/include/ios

681

Changing the size of basic_ios is a non-starter, because ABI.
However, we don't need a separate bool for this. Notice that eof() by definition is not equal to any possible value of char_type, so if __fill_ == traits_type::eof() then we can be 100% sure that it hasn't been set by the user. Certainly the scenario you describe in the PR summary — "the user sets a value for the fill character that happens to equal traits_type::eof()" — cannot possibly happen.

libcxx/test/std/input.output/iostreams.base/ios/basic.ios.members/fill_init.pass.cpp

26 ↗

(On Diff #412164)

If this 0 means a pointer, please prefer nullptr instead.

36 ↗

(On Diff #412164)

eof() is not a character, so you can't fill with it. AFAIK, this line has UB.

[ldionne]

libcxx/include/ios

681

This is ABI-breaking. I don't think we can fix that part of the bug. Do you know how other implementations do it? Do they use a boolean flag similarly to this? That seems like a rather strange implementation requirement to me -- is it possible that all implementations have the same bug? In that case, perhaps we only need a LWG issue to actually standardize what all implementations do, but I'm just speculating.

[Jake-Egan]

Thanks for the review. I removed the flag and new test. I modified the existing test for fill(char_type) to test the initialization of the fill character on the first call. This test was passing because it called fill() first in the assert. Calling fill() first is already tested in fill.pass.cpp

[• Quuxplusone]

LGTM now! but let's wait for @ldionne to re-examine it.

libcxx/include/ios

784–786

Ah, this looks vastly better!
At first I wondered whether these three lines should be replaced with

char_type __r = traits_type::eq_int_type(__fill_, traits_type::eof()) ? widen(' ') : __fill_;

so we're not overwriting __fill_ twice; but I see that what you've done here is consistent with lines 774-775 above, so IMHO it's fine as-is.

[Jake-Egan]

Rebase to hopefully fix format check

[ldionne]

LGTM if this passes CI. I have a hunch we might need some XFAIL: apple-macosxFOO markup if this ends up being instantiated in the dylib. If not, ship it.

Thanks for fixing!

libcxx/include/ios

protected:
     void move(basic_ios&& __rhs) {move(__rhs);}
     _LIBCPP_INLINE_VISIBILITY
     void swap(basic_ios& __rhs) _NOEXCEPT;
     _LIBCPP_INLINE_VISIBILITY
     void set_rdbuf(basic_streambuf<char_type, traits_type>* __sb);
 private:
     basic_ostream<char_type, traits_type>* __tie_;
     mutable int_type __fill_;
 };
 
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY
 basic_ios<_CharT, _Traits>::basic_ios(basic_streambuf<char_type,traits_type>* __sb)
 {
     init(__sb);
 }
 
basic_ios<_CharT, _Traits>::fill() const
     return __fill_;
 }
 
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY
 _CharT
 basic_ios<_CharT, _Traits>::fill(char_type __ch)
 {
+    if (traits_type::eq_int_type(traits_type::eof(), __fill_))
+        __fill_ = widen(' ');
     char_type __r = __fill_;
     __fill_ = __ch;
     return __r;
 }
 
 template <class _CharT, class _Traits>
 basic_ios<_CharT, _Traits>&
 basic_ios<_CharT, _Traits>::copyfmt(const basic_ios& __rhs)
 {

libcxx/test/std/input.output/iostreams.base/ios/basic.ios.members/fill_char_type.pass.cpp

 #include <ios>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**)
 {
     std::ios ios(0);
-    assert(ios.fill() == ' ');
     char c = ios.fill('*');
     assert(c == ' ');
     assert(ios.fill() == '*');
 
   return 0;
 }