[asterisk-dev] [Code Review] Merge Called party identification changes into trunk.

[rmudgett at digium.com]

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/include/asterisk/callerid.h, lines 422-424
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3248#file3248line422>
> >
> >     Please fully document all new API calls.

These functions are fully documented at the function definition.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/include/asterisk/channel.h, lines 2030-2044
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3249#file3249line2030>
> >
> >     Please document all new API calls.

These functions are fully documented at the function definition.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/include/asterisk/channel.h, lines 2046-2088
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3249#file3249line2046>
> >
> >     Please complete the documentation here to include parameters.

Fully documented at the function definition.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/callerid.c, line 1392
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3251#file3251line1392>
> >
> >     Personally, I find the comments placed at the end of switch statements, loops, and functions distracting.  This is not something that we have put in our formatting guielines, and none of the existing code uses it.
> >     
> >     If a lot of people agree that it's something we should start doing, then we could add it to the guidelines and merge this as is.  Otherwise, I'd rather all of it were removed.

These end of switch, loop, and function definition comments are useful when the beginning is several pages earlier.  They also make it easier when you want to add new code after a loop or switch and need to identify the curly brace when several blocks end at the same time.  Admittedly, they are not as useful when the item is short.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/channel.c, lines 1501-1513
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3252#file3252line1501>
> >
> >     Remove this commented out code if it is no longer needed.

This code is relevant for when the struct ast_callerid is broken up.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/channel.c, lines 6020-6032
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3252#file3252line6020>
> >
> >     remove commented out code

This code is relevant for when the struct ast_callerid is broken up.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/channel.c, lines 6074-6086
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3252#file3252line6074>
> >
> >     commented out code

This code is relevant for when the struct ast_callerid is broken up.


- rmudgett


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


On 2009-03-18 15:10:15, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/201/
> -----------------------------------------------------------
> 
> (Updated 2009-03-18 15:10:15)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This diff encompasses the changes between the issue8824 branch and Asterisk trunk. These changes include all the changes necessary to allow for the transmission and reception of remote called party identification (COLP/CONP).
> 
> 
> This addresses bug 8824.
>     http://bugs.digium.com/view.php?id=8824
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 182956 
>   /trunk/apps/app_dial.c 182956 
>   /trunk/apps/app_directed_pickup.c 182956 
>   /trunk/apps/app_queue.c 182956 
>   /trunk/channels/chan_agent.c 182956 
>   /trunk/channels/chan_dahdi.c 182956 
>   /trunk/channels/chan_h323.c 182956 
>   /trunk/channels/chan_iax2.c 182956 
>   /trunk/channels/chan_local.c 182956 
>   /trunk/channels/chan_mgcp.c 182956 
>   /trunk/channels/chan_misdn.c 182956 
>   /trunk/channels/chan_phone.c 182956 
>   /trunk/channels/chan_sip.c 182956 
>   /trunk/channels/chan_skinny.c 182956 
>   /trunk/channels/chan_unistim.c 182956 
>   /trunk/channels/misdn/chan_misdn_config.h 182956 
>   /trunk/channels/misdn/isdn_lib.h 182956 
>   /trunk/channels/misdn/isdn_lib.c 182956 
>   /trunk/channels/misdn/isdn_lib_intern.h 182956 
>   /trunk/channels/misdn/isdn_msg_parser.c 182956 
>   /trunk/channels/misdn_config.c 182956 
>   /trunk/configs/misdn.conf.sample 182956 
>   /trunk/configs/sip.conf.sample 182956 
>   /trunk/funcs/func_connectedline.c PRE-CREATION 
>   /trunk/funcs/func_redirecting.c PRE-CREATION 
>   /trunk/include/asterisk/callerid.h 182956 
>   /trunk/include/asterisk/channel.h 182956 
>   /trunk/include/asterisk/frame.h 182956 
>   /trunk/main/callerid.c 182956 
>   /trunk/main/channel.c 182956 
>   /trunk/main/dial.c 182956 
>   /trunk/main/features.c 182956 
>   /trunk/main/rtp.c 182956 
> 
> Diff: http://reviewboard.digium.com/r/201/diff
> 
> 
> Testing
> -------
> 
> Digium's Product Quality department has extensively tested a remarkably similar version of these enhancements. In addition, we know of a customer who has been using this branch for some months now in a production environment. I would also be willing to bet that some of those who have been monitoring issue 8824 have also done some testing as well.
> 
> 
> Thanks,
> 
> Mark
> 
>