Finishes implementation of
- P2093R14 Formatted output
- P2539R4 Should the output of std::print to a terminal be synchronized with the underlying stream?
Details
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Just skimmed over it. I do not know enough about std::print for an in-depth review, hence just a couple of typos
| libcxx/docs/ImplementationDefinedBehavior.rst | ||
|---|---|---|
| 18 | ||
| libcxx/include/__availability | ||
| 178 | ||
| libcxx/test/std/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/locale-specific_form.pass.cpp | ||
| 23 | There seems to be a verb missing... | |
| libcxx/include/ostream | ||
|---|---|---|
| 199–205 | I'd rather not start conditionally including headers, since that will make the dependencies we have even more complicated. | |
| 1278–1280 | Maybe use an __exception_guard instead? | |
| libcxx/include/print | ||
| 201–205 | This seems like a really useful thing to make it an extension to me. E.g. clang has -fcolor-diagnostics, which is essentially what this function is doing. | |
| libcxx/src/ostream.cpp | ||
| 21–32 | No need to make this unreadable. | |
| libcxx/src/std_stream.h | ||
| 115 | I think the inline changes here make sense as their own NFC patch. | |
Comment Actions
Thanks for the reviews!
| libcxx/include/ostream | ||
|---|---|---|
| 199–205 | Good point, that was part of a test and should have been unconditionally. | |
| 1278–1280 | I prefer to keep it as is now, this is consistent with the existing code, for example line 1228. Then make that change in one patch for the whole file, <ios> and <iomanip> | |
| libcxx/include/print | ||
| 201–205 | I'm not entirely sure what your suggestion is. Could you elaborate? | |
| libcxx/src/ostream.cpp | ||
| 21–32 | We use uglified code in other places in the dylib too. I prefer to keep it as is. When the code can be more from the dylib to headers there's no need to rename things. | |
| libcxx/src/std_stream.h | ||
| 115 | Agreed, I think it makes sense to just commit this when the CI is happy. | |
| libcxx/docs/ImplementationDefinedBehavior.rst | ||
|---|---|---|
| 14 | Also I think this entry is not sufficiently clear right now. You should explain what implementation-defined behavior you're talking about. Maybe provide a link to the section of the std you're talking about? | |
| libcxx/include/__availability | ||
| 177–178 | ||
| 180 | Missing _LIBCPP_AVAILABILITY_HAS_NO_FOO. | |
| 363 | Missing HAS_NO_FOO! | |
| libcxx/include/ostream | ||
|---|---|---|
| 154 | General question: do we follow this recommended practice? https://eel.is/c++draft/ostream.formatted.print#4
| |
| 1254 | ||
| 1276 | Given how long we discussed this line during live review, it might be worth explaining why this flush is correct. | |
| libcxx/include/print | ||
| 203 | Can you explain why this macro is necessary? How did you handle testing the previous print code without having to introduce this macro? | |
| libcxx/src/ostream.cpp | ||
| 24 | We should restore the -fno-rtti CI configuration that we seem to have dropped at some point, and then we can reconsider whether this approach is feasible: https://godbolt.org/z/WzEEz3TP1 IDK if this will be possible to implement without RTTI, since we can't add a new virtual function to basic_streambuf. | |
| libcxx/test/std/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp | ||
| 92 | Weird characters here! | |
| 183 | Can you link to the recommended practice here? https://eel.is/c++draft/ostream.formatted.print#4 | |
Comment Actions
Rebased and addresses parts of the review comments.
Note that Phabricator is quite unresponsive so skipped some comments.
| libcxx/modules/std/ostream.inc | ||
|---|---|---|
| 31–35 | I just skimmed over this patch out of curiosity. Shouldn't these be guarded against C++23 like in print.inc? | |