[asterisk-dev] [Code Review]: app_alarmreceiver cleanup and fixes

[rmudgett]

> On Aug. 20, 2012, 1:44 p.m., Mark Michelson wrote:
> > /trunk/apps/app_alarmreceiver.c, lines 478-497
> > <https://reviewboard.asterisk.org/r/2075/diff/3/?file=31019#file31019line478>
> >
> >     There are a few things I don't like here.
> >     
> >     First, I think you should pass the size of the event as a parameter to this function. This way there are fewer magic 16s being used in this function. I had to look a few times to make sure you could not possibly access data you should not be accessing.
> >     
> >     Second, you can declare the arrays without the size being specified (e.g. static char digit_map[] = "0123456789*#ABC";)
> >     
> >     Third, instead of using sizeof(digit_map) use ARRAY_LEN(digit_map). In this case, they evaluate to the same thing, but using ARRAY_LEN makes it much clearer that you mean the length of the array.
> >

You have to declare digit_map[15] = "0123456789*#ABC" because initializing it with a string would also includes a nul terminator in the array size.  Of course you could initialize it character by character.  Then you would not need the explicit array dimension.

Another way to declare this so you know what maps with what is:
struct {
  char digit;
  char weight;
} mapping[] = { {'0', 10 },... };


- rmudgett


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


On Aug. 17, 2012, 1:17 p.m., KNK wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2075/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2012, 1:17 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Review on behalf of Pedro Kiefer: cleanups from ASTERISK-20224, ASTERISK-20157 and the updated ASTERISK-16694 (author Fred van Lieshout) + improvement from ASTERISK-20158
> There are several issues opened for bugs in app_alarmreceiver (ASTERISK-19435, ASTERISK-18417, ASTERISK-16694 and ASTERISK-16668), which go away after the cleanup (ASTERISK-16694) replacing the proprietary toneburst code with ast_playtones_ functions, so the fix from ASTERISK-19435 is no longer necessary.
> The new functionality from ASTERISK-20158 is a bugfix on my opinion, because 'we get stuck on the alarm receiver app until it times out'
> 
> 
> This addresses bugs ASTERISK-16668, ASTERISK-16694, ASTERISK-18417, ASTERISK-19435, ASTERISK-20157, ASTERISK-20158, and ASTERISK-20224.
>     https://issues.asterisk.org/jira/browse/ASTERISK-16668
>     https://issues.asterisk.org/jira/browse/ASTERISK-16694
>     https://issues.asterisk.org/jira/browse/ASTERISK-18417
>     https://issues.asterisk.org/jira/browse/ASTERISK-19435
>     https://issues.asterisk.org/jira/browse/ASTERISK-20157
>     https://issues.asterisk.org/jira/browse/ASTERISK-20158
>     https://issues.asterisk.org/jira/browse/ASTERISK-20224
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_alarmreceiver.c 370760 
> 
> Diff: https://reviewboard.asterisk.org/r/2075/diff
> 
> 
> Testing
> -------
> 
> Compile test only
> 
> 
> Thanks,
> 
> KNK
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120820/542553d3/attachment.htm>