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

[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?

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

> +  class _format_cache_base;

"_format..." is not a reserved name.  It must be "__format..." 
or "_Format...".

> +    _format_cache_base*	_M_fc;

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?

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

> +  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?)

Is the conversion operator necessary?  Used?  Conversion operators
are not something to have without really strong reasons.

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

Nathan Myers
ncm@cantrip.org