[libcxx-commits] [PATCH] D63053: Add nullptr output operator overload (2221)

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 4 inline comments as done.
zoecarver added inline comments.


================
Comment at: include/ostream:219
     basic_ostream& operator<<(const void* __p);
+    basic_ostream& operator<<(nullptr_t);
     basic_ostream& operator<<(basic_streambuf<char_type, traits_type>* __sb);
----------------
EricWF wrote:
> We externally instantiate the rest of these methods, but we can't do that here (for backwards compat reasons).
> 
> You need to add `_LIBCPP_INLINE_VISIBILITY` and maybe put it at the bottom of the group because it's unique in this way.
Thanks, will do.


================
Comment at: include/ostream:714
+template<class _CharT, class _Traits>
+basic_ostream<_CharT, _Traits>&
+basic_ostream<_CharT, _Traits>::operator<<(nullptr_t)
----------------
EricWF wrote:
> `inline` or just define the thing inside the class.
I'll do the later. 


================
Comment at: include/ostream:717
+{
+    return *this << static_cast<const void*>(0x0);
+}
----------------
mclow.lists wrote:
> EricWF wrote:
> > `static_cast<const void*>(nullptr)`;
> > 
> > But I'm not sure we want to forward to the `const void*` overload. My understanding of that overload is that it exists to print "bool" values for types like `ostream` that used to implement `if (stream)` via a conversion to `void*`. 
> > 
> > I think we may just want to print `nill`.
> I suggested this approach to Zoe, because I thought that that was what GCC did. (print `0x0`)
> Turns out that I was wrong; GCC prints `nullptr` (at least that's what wandbox claims about gcc 9).
> 
> 
Alright. I like printing `nullptr` better anyway, so I will do that if it's OK with both of you. 


================
Comment at: test/std/input.output/iostream.format/output.streams/ostream.formatted/ostream.inserters/streambuf.pass.cpp:74
+        os << nullptr;
+        assert(sb.str().c_str()[0] == '0');
+    }
----------------
mclow.lists wrote:
> EricWF wrote:
> > I don't think this test is portable. Implementations can print `null` or `nill` or "hubbabalo" for that matter.
> > We should probably just make this a `LIBCPP_ASSERT`. And maybe we should also assert the string isn't empty (for when other STL's use our test suite).
> Agreed.
Will do.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D63053