[asterisk-dev] Nested Function Usage (twilson: branch 1.4 r153095 - in /branches/1.4: apps/ include/asterisk/ res/)

[Russell Bryant]

SVN commits to the Digium repositories wrote:
> Author: twilson
> Date: Fri Oct 31 10:45:29 2008
> New Revision: 153095
> 
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=153095
> Log:
> Recent CDR fixes moved execution of the 'h' exten into the bridging code, so variables that were set after ast_bridge_call was called would not show up in the 'h' exten.  Added a callback function to handle setting variables, etc. from w/in the bridging code.  Calls back into a nested function within the function calling ast_bridge_call
> 
> (closes issue #13793)
> Reported by: greenfieldtech
> 
> Modified:
>     branches/1.4/apps/app_dial.c
>     branches/1.4/include/asterisk/channel.h
>     branches/1.4/res/res_features.c
> 
> Modified: branches/1.4/apps/app_dial.c
> URL: http://svn.digium.com/view/asterisk/branches/1.4/apps/app_dial.c?view=diff&rev=153095&r1=153094&r2=153095
> ==============================================================================
> --- branches/1.4/apps/app_dial.c (original)
> +++ branches/1.4/apps/app_dial.c Fri Oct 31 10:45:29 2008
> @@ -1434,9 +1434,9 @@
>  		/* almost done, although the 'else' block is 400 lines */
>  	} else {
>  		const char *number;
> -		time_t end_time, answer_time = time(NULL);
>  
>  		strcpy(status, "ANSWER");
> +		pbx_builtin_setvar_helper(chan, "DIALSTATUS", status);
>  		/* Ah ha!  Someone answered within the desired timeframe.  Of course after this
>  		   we will always return with -1 so that it is hung up properly after the 
>  		   conversation.  */
> @@ -1731,9 +1731,30 @@
>  				res = ast_dtmf_stream(chan,peer,dtmfcalling,250);
>  			}
>  		}
> -		
> +
>  		if (!res) {
>  			struct ast_bridge_config config;
> +
> +			auto void end_bridge_callback(void);
> +			void end_bridge_callback (void)
> +			{
> +				char buf[80];
> +				time_t end;
> +
> +				time(&end);
> +
> +				ast_channel_lock(chan);
> +				if (chan->cdr->answer.tv_sec) {
> +					snprintf(buf, sizeof(buf), "%ld", end - chan->cdr->answer.tv_sec);
> +					pbx_builtin_setvar_helper(chan, "ANSWEREDTIME", buf);
> +				}
> +
> +				if (chan->cdr->start.tv_sec) {
> +					snprintf(buf, sizeof(buf), "%ld", end - chan->cdr->start.tv_sec);
> +					pbx_builtin_setvar_helper(chan, "DIALEDTIME", buf);
> +				}
> +				ast_channel_unlock(chan);
> +			}
>  
>  			memset(&config,0,sizeof(struct ast_bridge_config));
>  			if (play_to_caller)

I do not have any specific comments about this code, as it already went 
through a review on reviewboard.

However, this did bring a question up in mind.  Before the use of nested 
functions spreads much further, I would like to decide where to draw the 
line as far as when using a nested function is appropriate, and when a 
new function should just be created.  Once we decide on the guideline 
for its usage, I'd like to add it to our coding guidelines.

I need to get my mind straight on when this is a good thing for when I'm 
reviewing code that is coming through.  :)

Here is a start.  Add/remove with your thoughts as necessary.

----------

Nested functions are allowed, but their usage should be limited.  They 
are useful when you have a callback that performs a trivial operation 
(not very much code), but needs to access local variables in another 
stack frame.  Essentially, the benefit is just saving you from having to 
pack up a bunch of arguments in a struct to pass in as an argument.

-- 
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.