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

[Nathan Myers <ncm-nospam at cantrip dot org>]

On Tue, Jan 21, 2003 at 10:14:21PM -0500, Jerry Quinn wrote:
> From: Nathan Myers <ncm-nospam at cantrip dot org>
> > On Tue, Jan 21, 2003 at 01:05:52AM -0500, Jerry Quinn wrote:
> > > +      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?
 
Two things.  First, if the new or the constructor throws, maybe there's 
some cleanup to do for other class members?  Second, if _M_init throws, 
you have to dispose of __fc, too, before rethrowing.
  
> > > +  // 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.
> 	...
 
OK, let's count on Gaby to clean that up.
  

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

If some code in an ios_base member function wanted to look at the
cache, it wouldn't be able to see any of the members in the derived 
version.  (It doesn't know _CharT.)

Maybe there's no need for _Format_cache_base, and _M_format_cache can 
just go in basic_ios<>.  Maybe just put a dummy pointer in ios_base, in 
case you decide later you need to use it and don't want to break the 
ABI.  That is, use separate cache structs for ios_base and basic_ios<>.  

Also, please add a couple of dummy virtual functions in each of
ios_base and basic_ios<>.  I have a feeling we will want to use
them later when we do not want to break the ABI.

The only thing I know of to put in an ios_base format cache, at the
moment, is analyses of flags/precision/width settings.  Example, a 
flag to say that the radix has been determined to be decimal, or one 
to say that precision, width, and radix are all set to the defaults.

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

Please do.

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

Accessibility doesn't worry me, just maintenance risk.  One of the rules
of cache design is that you keep it as simple as possible, because caches 
are inherently fragile, and you discard it at the least provocation.  

I personally doubt that the speed difference of the assignment operator 
is enough to earn its keep.  It's a debatable point in this case, though, 
and I won't press it.

Nathan Myers
ncm-nospam@cantrip.org