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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

In D116642#3225747 <https://reviews.llvm.org/D116642#3225747>, @Quuxplusone wrote:

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

Thanks! The version is `14.0`, right?



================
Comment at: libcxx/include/__filesystem/directory_entry.h:26
 #include <cstdlib>
+#include <ostream>
 #include <system_error>
----------------
Quuxplusone wrote:
> 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>`.
Thanks for the explanation, changed to `<iosfwd>`.


================
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>();
----------------
Quuxplusone wrote:
> 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?
Yes, I think it's a bug (it happens with all sized character types and even `unsigned char`). Created [a GitHub issue](https://github.com/llvm/llvm-project/issues/53119) and referenced it in comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116642