This is an archive of the discontinued LLVM Phabricator instance.

[libc++][print] Adds ostream overloads.
AbandonedPublic

Authored by Mordante on Jul 30 2023, 3:41 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

Finishes implementation of

  • P2093R14 Formatted output
  • P2539R4 Should the output of std::print to a terminal be synchronized with the underlying stream?

Diff Detail

Event Timeline

Mordante created this revision.Jul 30 2023, 3:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2023, 3:41 AM
Mordante updated this revision to Diff 545419.Jul 30 2023, 4:05 AM

CI fixes.

Mordante updated this revision to Diff 545426.Jul 30 2023, 5:22 AM

CI fixes.

Mordante updated this revision to Diff 545442.Jul 30 2023, 7:39 AM

CI fixes.

Mordante updated this revision to Diff 545448.Jul 30 2023, 9:34 AM

CI fixes.

Mordante updated this revision to Diff 545455.Jul 30 2023, 11:03 AM

CI testing.

Mordante updated this revision to Diff 545458.Jul 30 2023, 11:16 AM

Reenable CI.

Mordante updated this revision to Diff 546077.Aug 1 2023, 8:21 AM

CI fixes.

Mordante updated this revision to Diff 546118.Aug 1 2023, 10:13 AM

CI fixes.

Mordante updated this revision to Diff 546129.Aug 1 2023, 10:41 AM

CI fixes.

Mordante updated this revision to Diff 546151.Aug 1 2023, 11:02 AM

CI fixes.

Mordante updated this revision to Diff 546170.Aug 1 2023, 11:57 AM

CI fixes.

Mordante updated this revision to Diff 546186.Aug 1 2023, 12:26 PM

Reenable complete CI

Mordante updated this revision to Diff 546490.Aug 2 2023, 8:23 AM

Rebased to trigger CI.

Mordante published this revision for review.Aug 2 2023, 12:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2023, 12:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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
138
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...

philnik added a subscriber: philnik.Aug 4 2023, 1:14 PM
philnik added inline comments.
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.

Mordante marked 5 inline comments as done.Aug 12 2023, 4:11 AM

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.

Mordante updated this revision to Diff 549592.Aug 12 2023, 4:12 AM
Mordante marked 2 inline comments as done.

Rebased and addresses review comments.

ldionne added a subscriber: ldionne.Sep 5 2023, 8:40 AM
ldionne added inline comments.
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
137–138
140

Missing _LIBCPP_AVAILABILITY_HAS_NO_FOO.

247

Missing HAS_NO_FOO!

ldionne requested changes to this revision.Sep 6 2023, 9:45 AM
ldionne added inline comments.
libcxx/include/ostream
154

General question: do we follow this recommended practice? https://eel.is/c++draft/ostream.formatted.print#4

Recommended practice: For vprint_unicode, if invoking the native Unicode API requires transcoding, implementations should substitute invalid code units with U+FFFD REPLACEMENT CHARACTER per the Unicode Standard, Chapter 3.9 U+FFFD Substitution in Conversion.

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

This revision now requires changes to proceed.Sep 6 2023, 9:45 AM
Mordante marked 10 inline comments as done.Sep 10 2023, 9:51 AM
Mordante updated this revision to Diff 556374.Sep 10 2023, 9:55 AM

Rebased and addresses parts of the review comments.
Note that Phabricator is quite unresponsive so skipped some comments.

Zingam added a subscriber: Zingam.Sep 13 2023, 2:18 PM
Zingam added inline comments.
libcxx/modules/std/ostream.inc
32–36

I just skimmed over this patch out of curiosity. Shouldn't these be guarded against C++23 like in print.inc?

Mordante updated this revision to Diff 557941.Oct 30 2023, 12:52 PM
Mordante marked 2 inline comments as done.

Rebased and partly addresses review comments.

Mordante added inline comments.Oct 30 2023, 12:55 PM
libcxx/include/ostream
154

Yes we do, this is tested in earlier patches
test/libcxx/input.output/iostream.format/print.fun/transcoding.pass.cpp
test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp

libcxx/include/print
203

In the other tests I found ways around by calling implementation specific functions from test/libcxx. Here that's not possible.

libcxx/modules/std/ostream.inc
32–36

Yes they should. When I wrote this patch we didn't allow the std module in C++20 so it was not needed. But now we support them in C++20, thanks for catching this.

Mordante updated this revision to Diff 558148.Wed, Nov 22, 10:52 AM

Undoes changes to __availability that were a badly resolved merge conflict. This gave build issues with GCC.
Rebased.

Mordante abandoned this revision.Thu, Nov 23, 11:56 AM

Since the CI for Phabricator is turned off this review has been replaced by https://github.com/llvm/llvm-project/pull/73262