On Tue, May 6, 2025 at 2:07 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> On Tue, 6 May 2025 at 12:42, Tomasz Kaminski <tkami...@redhat.com> wrote:
> >
> >
> >
> > On Tue, May 6, 2025 at 1:34 PM Jonathan Wakely <jwak...@redhat.com>
> wrote:
> >>
> >> On 05/05/25 14:44 +0200, Tomasz Kamiński wrote:
> >> >This patch provides an equality operator between _Sink_iter and
> default_sentinel,
> >> >that returns true, if any further writes to the _Sink_iter and
> underlying _Sink,
> >> >will be discared, and thus can be omitted. This operator is
> implemented in terms
> >> >new of _M_discarding virtual function in _Sink.
> >> >
> >> >Currently only the _Padding_sink reports discarding mode of if width
> of sequence
> >> >characters is greater than _M_maxwidth (precision), or underlying
> _Sink is
> >> >discarding characters. The _M_discarding override, is separate
> function from
> >> >_M_ignoring, that remain annotated with [[__gnu__::__always_inline__]].
> >> >
> >> >Despite having notion of maximum characters to be written (_M_max),
> _Iter_sink
> >> >nevers discard characters, as the total number of characters that
> would be written
> >> >needs to be returned by format_to_n. This is documented in-source by
> providing an
> >> >_Iter_sink::_M_discarding override, that always returns false.
> >> >
> >> >The function is currently queried only by the _Padding_sinks, that may
> be stacked
> >> >for example a range is formatted, with padding with being specified
> both for range
> >> >itself and it's elements. The state of underlying sink is checked
> during construction
> >> >and after each write (_M_sync_discarding).
> >> >
> >> >libstdc++-v3/ChangeLog:
> >> >
> >> > * include/std/format (__Sink_iter<_CharT>::operator==)
> >> > (__Sink<_CharT>::_M_discarding, _Iter_sink<_CharT,
> _OutIter>::_M_discarding)
> >> > (_Padding_sinl<_CharT, _Out>::_M_padwidth)
> >> > (_Padding_sink<_CharT, _Out>::_M_maxwidth): Remove const.
> >> > (_Padding_sink<_CharT, _Out>::_M_sync_discarding)
> >> > (_Padding_sink<_CharT, _Out>::_M_discarding): Define.
> >> > (_Padding_sink<_CharT, _Out>::_Padding_sink(_Out, size_t,
> size_tt))
> >> > (_Padding_sink<_CharT, _Out>::_M_force_update):
> >> > (_Padding_sink<_CharT, _Out>::_M_flush): Call
> _M_sync_discarding.
> >> > (_Padding_sink<_CharT, _Out>::_Padding_sink(_Out, size_t)):
> Delegate.
> >> >---
> >> > libstdc++-v3/include/std/format | 66 +++++++++++++++++++++++++++------
> >> > 1 file changed, 54 insertions(+), 12 deletions(-)
> >> >
> >> >diff --git a/libstdc++-v3/include/std/format
> b/libstdc++-v3/include/std/format
> >> >index 054ce350440..08a28fc92da 100644
> >> >--- a/libstdc++-v3/include/std/format
> >> >+++ b/libstdc++-v3/include/std/format
> >> >@@ -3144,6 +3144,10 @@ namespace __format
> >> > auto
> >> > _M_reserve(size_t __n) const
> >> > { return _M_sink->_M_reserve(__n); }
> >> >+
> >> >+ friend bool
> >> >+ operator==(_Sink_iter __it, default_sentinel_t)
> >> >+ { return __it._M_sink->_M_discarding(); }
> >>
> >> I don't love using default_sentinel like this.
> >>
> >> Conceptually an output iterator that compares equal to a sentinel but
> >> is still writable seems ... odd.
> >
> > For me this didn't feel odd, just what I would intuitively think of
> "safe" output iterator,
> > that discards writes after bounds are reached.
>
> Yes, and this is purely an internal type, not user-facing, so it's
> reasonable to rely on the guarantee that writing past the sentinel is
> not a bug, just discarded.
>
User defined formatters see this type, when they retrieve iterators from
format context, so they are exposed to this comparison. This is why I
used part of the iterator interface in the first place.
>
> >>
> >> Did you choose this API just to avoid
> >> introducing a member function that isn't part of the usual iterator
> >> API?
> >
> > Yes, but I perceived this as something natural, so I just went for it.
> > I can just expose _M_discarding member in the _Sink_iter.
>
>
> Let's wait for comments from anybody else, maybe others like your
> solution more than I do. I don't hate it, I just wonder if it's a bit
> too "clever" and so isn't obvious to readers. But maybe others will
> disagree.
>
After seeing your comment, I think a different sentinel type to represent
that would
be a much cleaner design, like:
struct discarding_sentinel
{
constexpr discarding_sentinel(default_sentinel_t); // o op== default
sentinel works.
};
But we do not need to go extra here, so I will just have _M_discarding here:
>
>
> >>
> >>
> >> > };
> >> >
> >> > // Abstract base class for type-erased character sinks.
> >> >@@ -3263,6 +3267,11 @@ namespace __format
> >> > _M_bump(size_t __n)
> >> > { _M_next += __n; }
> >> >
> >> >+ // Returns true if the _Sink is discarding incoming characters.
> >> >+ virtual bool
> >> >+ _M_discarding() const
> >> >+ { return false; }
> >> >+
> >> > public:
> >> > _Sink(const _Sink&) = delete;
> >> > _Sink& operator=(const _Sink&) = delete;
> >> >@@ -3488,6 +3497,15 @@ namespace __format
> >> > _M_count += __s.size();
> >> > }
> >> >
> >> >+ bool
> >> >+ _M_discarding() const override
> >> >+ {
> >> >+ // format_to_n return total number of characters, that would be
> written,
> >> >+ // thus we need to see and all characters.
> >>
> >> "see and all characters" seems wrong, does it mean "see and format all
> >> characters"? Or somethign else?
> >>
> >> >+ // see https://eel.is/c%2B%2Bdraft/format#functions-22
> >>
> >> These links aren't necessarily permanent, so I would prefer a "dated"
> >> reference to a particular standard (or working paper), e.g.
> >> // see C++20 [format.functions] p20
> >>
> >> N.B. it was p20 in C++20, but p22 in C++23, so if you prefr to link to
> >> the newer standard, then:
> >>
> >> // see C++23 [format.functions] p22
> >>
> >> >+ return false;
> >> >+ }
> >> >+
> >> > public:
> >> > [[__gnu__::__always_inline__]]
> >> > explicit
> >> >@@ -3550,6 +3568,15 @@ namespace __format
> >> > }
> >> > }
> >> >
> >> >+ bool
> >> >+ _M_discarding() const override
> >> >+ {
> >> >+ // format_to_n return total number of characters, that would be
> written,
> >> >+ // thus we need to see and all characters.
> >> >+ // see https://eel.is/c%2B%2Bdraft/format#functions-22
> >>
> >> Same comment about "see and all" and the standard reference.
> >>
> >> >+ return false;
> >> >+ }
> >> >+
> >> > typename _Sink<_CharT>::_Reservation
> >> > _M_reserve(size_t __n) final
> >> > {
> >> >@@ -3636,17 +3663,15 @@ namespace __format
> >> > template<typename _Out, typename _CharT>
> >> > class _Padding_sink : public _Str_sink<_CharT>
> >> > {
> >> >- const size_t _M_padwidth;
> >> >- const size_t _M_maxwidth;
> >> >+ size_t _M_padwidth;
> >> >+ size_t _M_maxwidth;
> >> > _Out _M_out;
> >> > size_t _M_printwidth;
> >> >
> >> > [[__gnu__::__always_inline__]]
> >> > bool
> >> > _M_ignoring() const
> >> >- {
> >> >- return _M_printwidth >= _M_maxwidth;
> >> >- }
> >> >+ { return _M_printwidth >= _M_maxwidth; }
> >> >
> >> > [[__gnu__::__always_inline__]]
> >> > bool
> >> >@@ -3659,12 +3684,21 @@ namespace __format
> >> > return false;
> >> > }
> >> >
> >> >+ void
> >> >+ _M_sync_discarding()
> >> >+ {
> >> >+ if constexpr (is_same_v<_Out, _Sink_iter<_CharT>>)
> >> >+ if (_M_out == default_sentinel)
> >> >+ _M_maxwidth = _M_printwidth;
> >> >+ }
> >> >+
> >> > void
> >> > _M_flush()
> >> > {
> >> > span<_CharT> __new = this->_M_used();
> >> > basic_string_view<_CharT> __str(__new.data(), __new.size());
> >> > _M_out = __format::__write(std::move(_M_out), __str);
> >> >+ _M_sync_discarding();
> >> > this->_M_rewind();
> >> > }
> >> >
> >> >@@ -3682,7 +3716,10 @@ namespace __format
> >> > // We have more characters than padidng, no padding is needed,
> >> > // write direclty to _M_out.
> >> > if (_M_printwidth >= _M_padwidth)
> >> >- _M_out = __format::__write(std::move(_M_out), __str);
> >> >+ {
> >> >+ _M_out = __format::__write(std::move(_M_out), __str);
> >> >+ _M_sync_discarding();
> >> >+ }
> >> > // We reached _M_maxwidth that is smaller than _M_padwidth.
> >> > // Store the prefix sequence in _M_seq, and free _M_buf.
> >> > else
> >> >@@ -3718,6 +3755,10 @@ namespace __format
> >> > _Str_sink<_CharT>::_M_overflow();
> >> > }
> >> >
> >> >+ bool
> >> >+ _M_discarding() const override
> >> >+ { return _M_ignoring(); }
> >> >+
> >> > typename _Sink<_CharT>::_Reservation
> >> > _M_reserve(size_t __n) override
> >> > {
> >> >@@ -3752,15 +3793,16 @@ namespace __format
> >> >
> >> > public:
> >> > [[__gnu__::__always_inline__]]
> >> >- explicit _Padding_sink(_Out __out, size_t __padwidth)
> >> >- : _M_padwidth(__padwidth), _M_maxwidth(-1),
> >> >+ explicit
> >> >+ _Padding_sink(_Out __out, size_t __padwidth, size_t __maxwidth)
> >> >+ : _M_padwidth(__padwidth), _M_maxwidth(__maxwidth),
> >> > _M_out(std::move(__out)), _M_printwidth(0)
> >> >- { }
> >> >+ { _M_sync_discarding(); }
> >> >
> >> > [[__gnu__::__always_inline__]]
> >> >- explicit _Padding_sink(_Out __out, size_t __padwidth, size_t
> __maxwidth)
> >> >- : _M_padwidth(__padwidth), _M_maxwidth(__maxwidth),
> >> >- _M_out(std::move(__out)), _M_printwidth(0)
> >> >+ explicit
> >> >+ _Padding_sink(_Out __out, size_t __padwidth)
> >> >+ : _Padding_sink(std::move(__out), __padwidth, (size_t)-1)
> >> > { }
> >> >
> >> > _Out
> >> >--
> >> >2.49.0
> >> >
> >> >
> >>
>
>
