[libcxx-commits] [PATCH] D120751: [libc++] Fix initialization of __fill_

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/ios:681
     mutable int_type __fill_;
+    bool __fill_set;
 };
----------------
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.


================
Comment at: libcxx/test/std/input.output/iostreams.base/ios/basic.ios.members/fill_init.pass.cpp:26
+  {
+    std::basic_ios<char> ios(0);
+    assert(ios.fill() == ' ');
----------------
If this `0` means a pointer, please prefer `nullptr` instead.


================
Comment at: libcxx/test/std/input.output/iostreams.base/ios/basic.ios.members/fill_init.pass.cpp:36
+    ios.fill('x');
+    char ch = ios.fill(std::char_traits<char>::eof());
+    assert(ch == 'x');
----------------
`eof()` is not a character, so you can't fill with it. AFAIK, this line has UB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120751