[asterisk-dev] [Code Review] Update timing to deal with multiple ranges and per-minute accuracy

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/53/#review137
-----------------------------------------------------------



trunk/main/pbx.c
<http://reviewboard.digium.com/r/53/#comment244>

    This isn't anything new introduced by your patch, but I am of the opinion that the variables in this function have bad names. All of the one-letter variable names could stand to be renamed.



trunk/main/pbx.c
<http://reviewboard.digium.com/r/53/#comment242>

    Good optimization. I like it :)



trunk/main/pbx.c
<http://reviewboard.digium.com/r/53/#comment245>

    Once again, nothing new introduced with your patch, but it seems odd that lookup_name returns the index of the array +1, meaning that you always have to decrement the return value. I think it would be better to return the actual array index in lookup_name and return -1 on an error condition.



trunk/main/pbx.c
<http://reviewboard.digium.com/r/53/#comment243>

    There isn't anything new wrong here introduced with your patch, but in the case that someone specifies a range which wraps around (like fri-mon), then the LSB of the mask will get set twice as a result. This is because s is not incremented in the case when it is greater than or equal to the max. If the s++ statement were moved to after the if-else block (i.e. at the very end of the while block), then this extra iteration would be avoided.



trunk/main/pbx.c
<http://reviewboard.digium.com/r/53/#comment246>

    I think that instead of "times" the variable "a" should be used here instead.



trunk/main/pbx.c
<http://reviewboard.digium.com/r/53/#comment247>

    I'm not sure that this range checking is necessary any more since you checked the time values earlier when you scanned them in.


In general, don't forget to add notes to the CHANGES file noting that you can use '&' to specify non-contiguous times, that it operates at a 1-minute resolution now instead of 2-minute, and also that a range is no longer required when specifying the time of day.

- Mark


On 2008-11-12 17:03:40, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/53/
> -----------------------------------------------------------
> 
> (Updated 2008-11-12 17:03:40)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The timing API was mis-documented in TFOT to allow multiple ranges and single hours, through the use of the '&'.  This patch updates the code to make TFOT correct.  Also, it sets times to be accurate down to the minute, instead of the current 2-minute precision that confuses so many people.
> 
> 
> Diffs
> -----
> 
>   trunk/include/asterisk/pbx.h 156387 
>   trunk/main/pbx.c 156387 
> 
> Diff: http://reviewboard.digium.com/r/53/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Tilghman
> 
>