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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % comments; please wait for a second green light before landing.



================
Comment at: libcxx/test/std/experimental/iterator/ostream.joiner/ostream.joiner.ops/ostream_joiner.op.assign.pass.cpp:93
+    // TODO(var-const): uncomment when it becomes possible to instantiate a `basic_ostream` object with a sized
+    // character type (see https://github.com/llvm/llvm-project/issues/53119).
 //  test<char, const char *, char16_t>('X', chars, chars+10, u"0X1X2X3X4X5X6X7X8X9");
----------------
Quuxplusone wrote:
> var-const wrote:
> > Quuxplusone wrote:
> > > Please use https://llvm.org/PRxxxxx URLs.
> > Can you please elaborate a little? First of all, which patch do I use? IIUC, libc++ bugs are reported on GitHub, so I'm not sure which patch should be used here. Second, why is a link to a patch preferable to a link to a GitHub issue?
> https://llvm.org/PR53119 . In LLVM jargon, ["PR" stands for "problem report"](https://quuxplusone.github.io/blog/2019/08/02/the-tough-guide-to-cpp-acronyms/#pr) (or some such). The benefit of `llvm.org` URLs over `github.com` URLs is that we control the former, so they're less likely to bit-rot ten years from now.
This hasn't been done yet.


================
Comment at: libcxx/test/std/input.output/filesystems/class.directory_entry/directory_entry.io/directory_entry.io.pass.cpp:36
+  const CharT* expected_output = static_cast<const CharT*>(OutStr);
+  directory_entry dir{path(input)};
+  std::basic_stringstream<CharT> stream;
----------------
Consider const-qualifying (just to prove that `operator<<`'s operand is const-qualified), and prefer parens-init to brace-init for non-aggregates:
```
const fs::directory_entry dir = fs::directory_entry(fs::path(input));
```
(I also think this is the only line that needs `fs::` qualifiers, so if you adopt the above suggestion, you can remove lines 32-33 entirely.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116642