[asterisk-dev] [Code Review]: dsp.c ast_dsp_call_progress use local short variable in loop, plus other cleanup

[Alec Davis]

> On Sept. 14, 2012, 11:32 a.m., rmudgett wrote:
> > branches/1.8/main/dsp.c, line 511
> > <https://reviewboard.asterisk.org/r/2107/diff/2/?file=31198#file31198line511>
> >
> >     Chaining inits this way just makes for long lines.

I was wary of line length, only 84 in this case.

So maybe in ast_dsp_digitreset() the chained init, which I copy and pasted should also be 2 lines. hits[]=0 on one line, and current_hit=0 on another.
In case current_hit becomes a different type that hits[] in the future.


> On Sept. 14, 2012, 11:32 a.m., rmudgett wrote:
> > branches/1.8/main/dsp.c, line 494
> > <https://reviewboard.asterisk.org/r/2107/diff/2/?file=31198#file31198line494>
> >
> >     Chaining inits this way just makes for long lines.
> >     
> >     Also if the types are different, you may set yourself up for subtle bugs because of conversions.

regarding chaining inits, I'd checked they were the same type.
But in the future one may change, so yes I agree perhaps should be on seperate lines.

Actually chained inits make for more code;
 483:dsp.c         ****         s->lasthit = 0;
 13486                  .loc 1 483 0
 13487 0500 8B4508      movl 8(%ebp),%eax
 13488 0503 C7809000    movl $0,144(%eax)
 13488      00000000
 13488      0000
 484:dsp.c         ****         s->current_hit = 0;
 13489                  .loc 1 484 0
 13490 050d 8B4508      movl 8(%ebp),%eax
 13491 0510 C7809400    movl $0,148(%eax)
 13491      00000000
 13491      0000
 485:dsp.c         ****         for (i = 0;  i < 4;  i++) {
 13492                  .loc 1 485 0
 13493 051a C745F400    movl $0,-12(%ebp)


 483:dsp.c         ****         s->lasthit = s->current_hit = 0;
 13486                  .loc 1 483 0
 13487 0500 8B4508      movl 8(%ebp),%eax
 13488 0503 C7809400    movl $0,148(%eax)
 13488      00000000
 13488      0000
 13489 050d 8B4508      movl 8(%ebp),%eax
 13490 0510 8B909400    movl 148(%eax),%edx
 13490      0000
 13491 0516 8B4508      movl 8(%ebp),%eax
 13492 0519 89909000    movl %edx,144(%eax)
 13492      0000
 484:dsp.c         ****         for (i = 0;  i < 4;  i++) {
 13493                  .loc 1 484 0
 13494 051f C745F400    movl $0,-12(%ebp)


- Alec


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2107/#review7066
-----------------------------------------------------------


On Sept. 12, 2012, 6:12 a.m., Alec Davis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2107/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2012, 6:12 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> janitor cleanup. No functional change.
> 
> 1). ast_dsp_call_progress: use 'short samp' instead of s[x] inside loop.
>     apply same casting as other _init, dsp->energy = (int32_t) samp * (int32_t) samp
> 
> 2). ast_dtmf_detect_init: move repeated setting of s->energy to outside of loop.
>     do goertzel_init loop first before setting s->lasthit and s->current_hit, consistant with ast_dsp_digitreset()
> 
> 3). ast_mf_detect_init:
>     do goertzel_init loop first before setting s->hits[] and s->current_hit, consistant with ast_dsp_digitreset()
> 
> 4). white space, in areas I've been recently, plus others.
> 
> 
> Diffs
> -----
> 
>   branches/1.8/main/dsp.c 372929 
> 
> Diff: https://reviewboard.asterisk.org/r/2107/diff
> 
> 
> Testing
> -------
> 
> compiles.
> 
> 
> Thanks,
> 
> Alec
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120914/9a2a3bd5/attachment.htm>