Page MenuHomePhabricator

[libc++][format][5/6] Improve format_to_n.
Needs ReviewPublic

Authored by Mordante on Sep 26 2021, 8:05 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Summary

Use a specialized buffer wrapper to limit the number of insertions in the
buffer. After the limit has been reached the buffer only needs to count
the number of insertions to return the buffer size required to store the
entire output.

Depends on D110498

Diff Detail

Event Timeline

Mordante requested review of this revision.Sep 26 2021, 8:05 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2021, 8:05 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/include/__format/buffer.h
192–193
195

Given that you continue incrementing __size_ even after hitting the end of __buffer_, I think it's a bit dangerous to use any type here except size_t. I recommend s/_Size/size_t/g.
I don't see any particular usefulness in continuing to store __n_ as _Size, either. formatted_size() is specced to return size_t, not iter_difference_t<anything>, so I think size_t throughout would make plenty of sense.

Mordante marked 2 inline comments as done.Sep 26 2021, 9:15 AM
Mordante added inline comments.
libcxx/include/__format/buffer.h
192–193

Again what clang-format makes of the code.

195

The normal formatted_size indeed returns a size_t, but format_to_n returns

template<class Out> struct format_to_n_result {
  Out out;
  iter_difference_t<Out> size; // This is the formatted_size.
};

So I prefer to keep this the same type as the Standard specified.

Mordante planned changes to this revision.Sep 26 2021, 10:00 AM
Mordante marked 2 inline comments as done.

Needs to be updated due to the removal of __put in D110495.

Mordante updated this revision to Diff 378455.Oct 9 2021, 8:07 AM

Rebased and adjusted to changes in __put.

Mordante updated this revision to Diff 380190.Oct 16 2021, 8:48 AM

Rebased to test with wchar_t changes.

Mordante planned changes to this revision.Nov 2 2021, 10:57 AM
Mordante updated this revision to Diff 387097.Sun, Nov 14, 8:46 AM

Rebased and make changes based on D112361.

vitaut added inline comments.Thu, Nov 25, 8:33 AM
libcxx/include/format
598

Similarly to other diffs, let's introduce __vformat_to_n and force inline format_to_n for bloat prevention to actually work.

600–607

Not sure if this branch is worth adding. __format_to_n_buffer should perform reasonably well for this case.

Mordante marked an inline comment as done.Thu, Nov 25, 11:36 AM
Mordante added inline comments.
libcxx/include/format
600–607

I need to guard against __n < 0 since the __n is used as an unsigned capacity in the buffer code. So I thought it was easiest to add the guard here.

vitaut added inline comments.Fri, Nov 26, 8:13 AM
libcxx/include/format
600–607

I understand that you need a check, my comment was that special casing 0 (and negative size) is probably not worth it. I'd suggest keeping it simpler and go through __format_to_n_buffer in all cases.

Mordante updated this revision to Diff 390419.Mon, Nov 29, 11:14 AM
Mordante marked an inline comment as done.

Addresses review comments.

Mordante marked 2 inline comments as done.Mon, Nov 29, 11:17 AM