[libcxx-commits] [PATCH] D116642: [libc++] Implement operator<< for filesystem::directory_entry.

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

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

Please mention `[LWG3171]` in the commit subject line, and update Cxx2bIssues.csv. Otherwise, seems reasonable to me % comments.



================
Comment at: libcxx/include/__filesystem/directory_entry.h:26
 #include <cstdlib>
+#include <ostream>
 #include <system_error>
----------------
var-const wrote:
> From what I've seen, other headers that implement `operator<<` include `<iosfwd>` -- at the very least, I couldn't find one that includes `<ostream>`. However, I'm not sure this is actually valid given that the code actually invokes a member function on the stream (`operator<<`) whereas `<iosfwd>` only contains a forward declaration of the class.
I believe it's technically OK, because `operator<<` is always a template: it's calling a member function on some `basic_ostream<C, T>`, but that's a dependent type, so the call doesn't need to be (can't be) checked beyond that.
I strongly recommend being consistent [;)] which means including `<iosfwd>` here, not `<ostream>`.


================
Comment at: libcxx/include/filesystem:48-51
+    // The fs.dir.entry.io operator is a friend of directory_entry.
+    template<class charT, class traits>
+      friend basic_ostream<charT, traits>&
+        operator<<(basic_ostream<charT, traits>& os, const directory_entry& d);
----------------
The extra `//` comment here was the red flag for me; but actually I think this entire diff is unnecessary. This `operator<<` is specified inside the body of `class directory_entry`; the body of `directory_entry` is not shown here; therefore this `operator<<` needn't be shown here.
So I believe no change is needed anywhere in this particular file.


================
Comment at: libcxx/test/std/input.output/filesystems/class.directory_entry/directory_entry.io/directory_entry.io.pass.cpp:50
+  // TODO(var-const): uncomment when it becomes possible to instantiate a `basic_ostream` object with a sized character
+  // type. Currently the instantiation would fail because `ctype` only has specializations for `char` and `wchar_t`.
+  //TestOutput<char8_t>();
----------------
var-const wrote:
> See https://wandbox.org/permlink/hUEUX67Ejcgxz6UT.
`path.io.pass.cpp` (c79795874adef) and `ostream_joiner.op.assign.pass.cpp` (e046bbdded2c6) seem to be the only places in the codebase where we bother with this kind of commented-out `char{8,16,32}_t` business. I'm not sure it's worth duplicating here... //but//, your example seems to indicate that libc++'s lack of support for `stringstream<char16_t>` is actually a bug? [libstdc++ and MSVC support char16_t streams reasonably well AFAICT](https://godbolt.org/z/7Kj8z3j7r). So perhaps this should be filed as a bug against libc++, and then you can explicitly mention https://llvm.org/PRxxxxx both here and in those two other places?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116642