Re: [patch] PR 8761 and 7076, take 3

[Jerry Quinn <jlquinn at optonline dot net>]

From: Nathan Myers <ncm-nospam at cantrip dot org>
> On Tue, Jan 21, 2003 at 01:05:52AM -0500, Jerry Quinn wrote:
> > OK, here is the next rendition of my patch for speeding up integer I/O 
> > output performance.
> 
> Woohoo!  Once we get the nits below picked, let's check it
> in and then work from that base.
> 
> > @@ -142,6 +149,12 @@
> >        // NB: This may be called more than once on the same object.
> >        ios_base::_M_init();
> >        _M_cache_facets(_M_ios_locale);
> > +      if (!_M_fc)
> > +	{
> > +	  _Format_cache<_CharT>* __fc = new _Format_cache<_CharT>();
> > +	  __fc->_M_init(*this);
> > +	  this->_M_fc = __fc;
> > +	}
> 
> Hmm, is that exception-safe?

If the new() fails, what is the right response?  As is, it will throw out the
library, right?  Or am I missing something?

 
> > +  template<typename _CharT, typename _Traits>
> > +    _Format_cache<_CharT>&
> > +    basic_ios<_CharT, _Traits>::_M_get_cache()
> > +    {
> > +      return *(_Format_cache<_CharT>*)_M_fc;
> > +    }
> > +  
> > +  template<typename _CharT, typename _Traits>
> > +    const _Format_cache<_CharT>&
> > +    basic_ios<_CharT, _Traits>::_M_get_cache() const
> > +    {
> > +      return *(_Format_cache<_CharT>*)_M_fc;
> > +    }
> 
> I can't tell if these are inline.  They should be, right?

Yeah, OK.
 
> > +  class _format_cache_base;
> 
> "_format..." is not a reserved name.  It must be "__format..." 
> or "_Format...".
> 
> > +    _format_cache_base*	_M_fc;

OK

> Nits:
> If I am not mistaken, _M_fc is always accessed through _M_get_cache.
> There seems no reason for its name to be very short.  Relatedly, does
> the "get_" in _M_get_cache help comprehension any?
> 
> > +    // Basic_ios stores the format cache, but sometimes, all we have is the
> > +    // ios_base.  This provides access and casting.  A little bit ugly as you
> > +    // must assign this to a _Format_cache<_CharT> for the cast to happen
> > +    // cleanly.  Not safe to call until _M_init() has happened.
> > +    _format_cache_base& _M_get_cache() { return *(_format_cache_base*)_M_fc; }
> 
> Is the cast needed?  Is the comment still accurate?

I missed that cast while cleaning up.  It was originally a void*.  I'll change
the accessor to _M_cache() and the ptr to _M_format_cache.  The comment is
still correct, in the sense that basic_ios::_M_init has happened at least
once, since that is where I'm constructing the cache object.  I can change the
comment to this.
 
> > +  // NB: Of the two parameters, _CharT can be deduced from the
> > +  // function arguments. The other (_Traits) has to be explicitly specified.
> > +
> > +  template<typename _CharT, typename _Traits>
> > +    struct __pad
> > +    {
> > +      static void
> > +      _S_pad(ios_base& __io, _CharT __fill, _CharT* __news, 
> > +	     const _CharT* __olds, const streamsize __newlen, 
> > +	     const streamsize __oldlen, const bool __num);
> > +    };
> 
> I don't see how arguments to a struct template can be deduced.  
> A better comment would explain why __pad is a struct at all.

I picked this up from Benjamin's mods to my first patch iteration.  It was
written this way before the patch, and was just relocated, comment and all.
Going back and looking at the code, I also don't understand why this is a struct.  

I scanned the Changelogs and found that this is where it was turned into a
struct + static member:

2002-07-30  Benjamin Kosnik  <bkoz@redhat.com>
	    Gabriel Dos Reis  <gdr@nerim.net>

	* include/bits/ostream.tcc: Change __pad to 
	__pad<_CharT, _Traits>::_S_pad. 
	* include/bits/locale_facets.h: Add __pad_traits generic and
	ostreambuf_iterator specialization.
	* include/bits/locale_facets.tcc: Change __pad into struct __pad
	with a _CharT and _Traits template parameter and _S_pad static
	member function.
	* src/locale-inst.cc: Update __pad instantiations.
	...

 
> > +  class _format_cache_base
> > +  {
> > +  public:
> > +    virtual
> > +    ~_format_cache_base() {}
> > +
> > +    template<typename _CharT>
> > +    operator _Format_cache<_CharT>*() { return (_Format_cache<_CharT>*)this; }
> > +  };
> 
> I'm surprised to find that _format_cache_base has no data members.
> (Maybe later?)

The cache has only two members that aren't template parameters.  My feeling
was that most of the time, you need at least one of the ones that is
parameterized, so all the fields might as well be there.  Also, isn't adding
fields to the end of a structure later safer than adding in the middle, which
would happen if you add fields to the base class?  Either way is fine by me.
 
> Is the conversion operator necessary?  Used?  Conversion operators
> are not something to have without really strong reasons.

It was a convenience.  I was trying to figure out a way to cast the base class
to the derived one without having to do it explicitly in the code wherever it
gets used.  I'll remove it if you prefer.

> > +  template<typename _CharT>
> > +    _Format_cache<_CharT>&
> > +    _Format_cache<_CharT>::operator=(const _Format_cache<_CharT>& __fc)
> > +    {
>  ...
> > +    }
> 
> When do we assign _Format_cache<> values?   Is it necessary?  Maybe 
> we can just leave the assignment operator private and undefined.
> (Or maybe not.  It just seems like an unnecessary maintenance risk to 
> have two functions that populate the cache.)

This is used during basic_ios::copyfmt.  It seemed silly to do all the facet
calls again, since the work is already done.  I could make the operator=()
protected and make basic_ios a friend of _Format_cache.  Then it's not
globally accessable, if that's a concern.


Jerry Quinn