[asterisk-dev] Re: [asterisk-commits] russell: trunk r48103 - /trunk/main/utils.c

[Luigi Rizzo]

On Wed, Nov 29, 2006 at 05:08:19AM -0000, asterisk-commits at lists.digium.com wrote:
> Author: russell
> Date: Tue Nov 28 23:08:19 2006
> New Revision: 48103
> 
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=48103
> Log:
> Remove an XXX command suggesting that this truncation should not be conditional,
> and also add a more verbose comment explaining why it is only needed in the
> case of appending to the string for any curious readers that come along in the
> future.

given the explaination, why aren't you truncating (i.e. resetting
the strnig to the initial value) unconditionally, anyways ?

Making it conditional is more work in all cases (not that i care,
but from what you wrote it sounds like that you were trying to
optimize the code).
But especially, and this is what i care about, it makes the behaviour
of the function harder to remember because of the different action
in the two cases.

Bottom line - unless the documented behaviour is
"in the !append case, on a failure the buffer will contain a partial result",
i see no reason to make the truncation conditional

cheers
luigi

> Modified:
>     trunk/main/utils.c
> 
> Modified: trunk/main/utils.c
> URL: http://svn.digium.com/view/asterisk/trunk/main/utils.c?view=diff&rev=48103&r1=48102&r2=48103
> ==============================================================================
> --- trunk/main/utils.c (original)
> +++ trunk/main/utils.c Tue Nov 28 23:08:19 2006
> @@ -997,7 +997,14 @@
>  			return AST_DYNSTR_BUILD_FAILED;
>  		(*buf)->len = need;
>  
> -		/* Truncate the previous attempt. XXX this should be unconditional */
> +		/* Truncate the previous attempt.  This is only needed for the
> +		 * append operation because if the write is from the beginning,
> +		 * it will get automatically overwritten when this function
> +		 * gets called again.  When appending, if this was not done,
> +		 * this function would be called over and over again, because
> +		 * each attempt would be appending more and more partial
> +		 * strings, never having enough space to finish.
> +		 */ 
>  		if (append)
>  			(*buf)->str[offset] = '\0';
>  
> 
> _______________________________________________
> --Bandwidth and Colocation provided by Easynews.com --
> 
> asterisk-commits mailing list
> To UNSUBSCRIBE or update options visit:
>    http://lists.digium.com/mailman/listinfo/asterisk-commits