Re: [Patch] Fix libstdc++/12439

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

Hi Paolo,

Sorry, I see problems with this patch.  Comments interspersed.


On Wed, Oct 01, 2003 at 03:00:08PM +0200, Paolo Carlini wrote:
> Tested x86-linux, if nobody objects will commit later today.
> 
> ///////////

> 2003-10-01  Paolo Carlini  <pcarlini@unitus.it>
> 
> 	PR libstdc++/12439
> 	* include/bits/locale_facets.tcc (time_put::put): Deal
> 	with the three issues pointed out by the PR.
> 	* testsuite/22_locale/time_put/put/char/12439_1.cc: New.
> 	* testsuite/22_locale/time_put/put/char/12439_3.cc: New.
> 	* testsuite/22_locale/time_put/put/wchar_t/12439_1.cc: New.
> 	* testsuite/22_locale/time_put/put/wchar_t/12439_2.cc: New.
> 	* testsuite/22_locale/time_put/put/wchar_t/12439_3.cc: New.
> diff -urN libstdc++-v3-orig/include/bits/locale_facets.tcc libstdc++-v3/include/bits/locale_facets.tcc
> --- libstdc++-v3-orig/include/bits/locale_facets.tcc	2003-09-30 13:38:47.000000000 +0200
> +++ libstdc++-v3/include/bits/locale_facets.tcc	2003-10-01 14:20:52.000000000 +0200
> @@ -2000,21 +2000,20 @@
>    template<typename _CharT, typename _OutIter>
>      _OutIter
>      time_put<_CharT, _OutIter>::
> -    put(iter_type __s, ios_base& __io, char_type, const tm* __tm, 
> +    put(iter_type __s, ios_base& __io, char_type __fill, const tm* __tm, 
>  	const _CharT* __beg, const _CharT* __end) const
>      {
>        locale __loc = __io.getloc();
>        ctype<_CharT> const& __ctype = use_facet<ctype<_CharT> >(__loc);
>        while (__beg != __end)
>  	{
> -	  char __c = __ctype.narrow(*__beg, 0);
> +	  const _CharT* __tmp = __beg;
>  	  ++__beg;
> -	  if (__c == '%')
> +	  if (__ctype.narrow(*__tmp, 0) == '%' && __beg != __end)
>  	    {
>  	      char __format;
>  	      char __mod = 0;
> -	      size_t __len = 1; 
> -	      __c = __ctype.narrow(*__beg, 0);
> +	      const char __c = __ctype.narrow(*__beg, 0);
>  	      ++__beg;
>  	      if (__c == 'E' || __c == 'O')
>  		{

First, why is this (still) calling narrow in a loop to identify
the '%' marker?  It should call widen once at the top, and compare 
the characters found against the widened version.  Likewise the 
'E' and 'O'.

Yes, I know the old code did it the slow way too.  Ultimately these 
widenings should be in the locale cache, so they need be called only 
once per program.  Generally, any instance of calling a facet member 
in a loop and not saving the result is likely to be a performance bug.

Performance bugs are undetectable in normal testing.  By some lights
that makes them worse than nonconformancies, because only detailed,
skilled inspection can locate them.  A FIXME comment turns a concealed
bug into a searchable one -- not as good as fixing it, but much better 
than ignoring it.


> @@ -2024,13 +2023,10 @@
>  		}
>  	      else
>  		__format = __c;
> -	      __s = this->do_put(__s, __io, _CharT(), __tm, __format, __mod);
> +	      __s = this->do_put(__s, __io, __fill, __tm, __format, __mod);
>  	    }
>  	  else
> -	    {
> -	      *__s = __c;
> -	      ++__s;
> -	    }
> +	    *__s++ = *__tmp;
>  	}
>        return __s;
>      }

This last delta is suboptimal.  Post-increment is quite expensive on 
many iterator types.  Code that takes general iterator types should
always use pre-increment.  Any use of post-increment where we don't 
know the iterator type should be considered a performance bug.

Nathan Myers
ncm-nospam@cantrip.org