[asterisk-dev] [Code Review] Opaquify remaining fields and move struct ast_channel into main/channel_internal_api.c

[rmudgett]

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



/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/1786/#comment10602>

    Make the function parameter const void *data to document what the function expects for the parameter.



/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/1786/#comment10598>

    It is inconsistent for the getter function to take a const struct ast_channel ptr and return a non-const pointer.



/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/1786/#comment10596>

    Put that space back in there.



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/1786/#comment10603>

    the ast_channel_destructor depends upon the stringfields being uninitialized until later so it can correctly destroy a partially initialized channel structure.  When you alloc and initialize the stringfields at the same time you break this!
    
    It is probably better to break the ast_channel_internal_alloc/ast_channel_internal_cleanup functions up into:
    ast_channel_internal_alloc()
    ast_channel_internal_stringfields_init()
    ast_channel_internal_stringfields_free()
    rather than redesign the channel constructor/destructor to handle partially setup channel structs.



/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/1786/#comment10604>

    Comment needs updating.



/trunk/main/channel_internal_api.c
<https://reviewboard.asterisk.org/r/1786/#comment10601>

    How picky do you want to be with the sort.  The fields below are not all sorted this way.
    
    Another sorting criteria could be access grouping for better cache performance.  Although, this criteria would be harder to identify.



/trunk/main/channel_internal_api.c
<https://reviewboard.asterisk.org/r/1786/#comment10605>

    blob



/trunk/main/channel_internal_api.c
<https://reviewboard.asterisk.org/r/1786/#comment10599>

    Put semicolons on the end of these lines so they don't look funny and give my editor a headache. :)



/trunk/main/channel_internal_api.c
<https://reviewboard.asterisk.org/r/1786/#comment10600>

    Put semicolons on the end of these lines so they don't look funny and give my editor a headache. :)



/trunk/main/channel_internal_api.c
<https://reviewboard.asterisk.org/r/1786/#comment10597>

    Using struct copies instead of memcpy is safer.  The compiler can catch type mismatches and won't screw up on the size.  At least it shouldn't screw up on the size. :)


- rmudgett


On March 1, 2012, 4:47 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1786/
> -----------------------------------------------------------
> 
> (Updated March 1, 2012, 4:47 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This should finish up the first stage of opaquification.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_channelredirect.c 357721 
>   /trunk/apps/app_chanspy.c 357721 
>   /trunk/apps/app_confbridge.c 357721 
>   /trunk/apps/app_dial.c 357721 
>   /trunk/apps/app_disa.c 357721 
>   /trunk/apps/app_dumpchan.c 357721 
>   /trunk/apps/app_externalivr.c 357721 
>   /trunk/apps/app_followme.c 357721 
>   /trunk/apps/app_macro.c 357721 
>   /trunk/apps/app_mixmonitor.c 357721 
>   /trunk/apps/app_parkandannounce.c 357721 
>   /trunk/apps/app_queue.c 357721 
>   /trunk/apps/app_speech_utils.c 357721 
>   /trunk/apps/app_stack.c 357721 
>   /trunk/apps/app_talkdetect.c 357721 
>   /trunk/bridges/bridge_multiplexed.c 357721 
>   /trunk/channels/chan_agent.c 357721 
>   /trunk/channels/chan_bridge.c 357721 
>   /trunk/channels/chan_dahdi.c 357721 
>   /trunk/channels/chan_local.c 357721 
>   /trunk/channels/chan_phone.c 357721 
>   /trunk/channels/chan_sip.c 357721 
>   /trunk/channels/chan_unistim.c 357721 
>   /trunk/channels/chan_vpb.cc 357721 
>   /trunk/channels/sig_analog.c 357721 
>   /trunk/include/asterisk/channel.h 357721 
>   /trunk/include/asterisk/channel_internal.h PRE-CREATION 
>   /trunk/main/app.c 357721 
>   /trunk/main/autoservice.c 357721 
>   /trunk/main/bridging.c 357721 
>   /trunk/main/channel.c 357721 
>   /trunk/main/channel_internal_api.c 357721 
>   /trunk/main/cli.c 357721 
>   /trunk/main/features.c 357721 
>   /trunk/main/file.c 357721 
>   /trunk/main/indications.c 357721 
>   /trunk/main/manager.c 357721 
>   /trunk/main/pbx.c 357721 
>   /trunk/main/rtp_engine.c 357721 
>   /trunk/res/res_agi.c 357721 
>   /trunk/res/res_musiconhold.c 357721 
>   /trunk/res/snmp/agent.c 357721 
> 
> Diff: https://reviewboard.asterisk.org/r/1786/diff
> 
> 
> Testing
> -------
> 
> unit tests pass. I think I ran the testsuite on it last night and it passed. I'm running it again right now just to make sure.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120309/805246d8/attachment-0001.htm>