Page MenuHomePhabricator

[libc++][format] Move iterators when needed.
ClosedPublic

Authored by Mordante on Oct 23 2021, 10:14 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rG91dd07235411: [libc++][format] Move iterators when needed.
Summary

LWG-3539 was already implemented but not marked as done.
LWG-3567 is implemented in this commit.

Diff Detail

Event Timeline

Mordante requested review of this revision.Oct 23 2021, 10:14 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2021, 10:15 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I think this needs a test case. But I also assume that libc++ hasn't yet done a lot of the work to enable move-only iterators, is that right? How big is that work, and do you think it would be reasonable to block this PR until that work gets done (so that this PR could be updated to include a test case involving a move-only output iterator)?

libcxx/include/__format/format_context.h
103–104

This is correct per the Standard, but also kinda crazy, in that y = x.out() modifies the value of x. It should have been specified with an rvalue-ref-qualifier, like out() &&, right?
No action required, anyway, since this PR implements what the Standard says to do. Just kibitzing.

Mordante planned changes to this revision.Oct 23 2021, 12:10 PM

I'll add tests. I think I can just use custom move only iterator and don't need to depend on move only iterators.
(In fact I expect the most common iterators for this class are copyable.)

libcxx/include/__format/format_context.h
103–104

I agree, this feels odd. But it's what the Standard mandates. I'm tempted to file an LWG-issue.

I spend some time implementing tests but see some other issues, which I want to investigate before proceeding with this patch.

libcxx/include/__format/format_context.h
103–104

I've decided to file an LWG-issue.

Mordante added inline comments.Sep 28 2022, 11:54 AM
libcxx/include/__format/format_context.h
103–104

I was writing an LWG issue, however I see some issues. The function out is called in the requirements tables http://eel.is/c++draft/tab:formatter.basic, which would require a call to std::move, the same for http://eel.is/c++draft/format.context#example-1.

So it might be confusing only the out_ member is moved-from, but arg can be called.

Do you still feel it's worth the effort to file an issue?

Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 11:54 AM
Mordante updated this revision to Diff 463642.Sep 28 2022, 12:01 PM

Rebased and a complete update of the patch.
Since parts of the format library now use ranges instead of pre C++20
algorithms internally it is possible to use move only iterators.

ldionne requested changes to this revision.Oct 11 2022, 9:19 AM
ldionne added inline comments.
libcxx/test/std/utilities/format/format.formatter/format.context/format.context/advance_to.pass.cpp
48

Can we not use cpp20_output_iterator<std::back_insert_iterator<...>> and avoid defining our own move_only_push_back_iterator?

libcxx/test/std/utilities/format/format.functions/format_to.pass.cpp
33–34

This looks like a merge conflict to me. I think you meant to mirror the changes in format_to.locale.pass.cpp?

This revision now requires changes to proceed.Oct 11 2022, 9:19 AM
Mordante marked 3 inline comments as done.Oct 12 2022, 11:56 AM

Thanks for the review!

Mordante updated this revision to Diff 467230.Oct 12 2022, 12:22 PM

Rebased and addresses review comments.

Mordante updated this revision to Diff 468005.Oct 15 2022, 3:47 AM

Readds removed header.

ldionne accepted this revision.Oct 18 2022, 8:36 AM
This revision is now accepted and ready to land.Oct 18 2022, 8:36 AM
This revision was automatically updated to reflect the committed changes.