[asterisk-dev] [Code Review] Continue ast_channel opaque-ification with accessor functions for the rest of the stringfields

[Matt Jordan]

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


I'm kind of against the macros, at least in this case.  In addition to what you've pointed out (lack of doxygen comments), they have two drawbacks, as I see them.

1) Not every field needs the variable arguments version of ast_string_field_set.  Things that don't need them, shouldn't have them.
2) By defining set accessors for every variable, we lose a lot of the encapsulation benefit that opaque-ifying ast_channel provides.  For example, even if there are places in the codebase that currently set ast_channel's uniqueid field - should they really be allowed to do so?

- Matt


On Jan. 9, 2012, 5:15 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1661/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2012, 5:15 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch converts the rest of the stringfields to setters/getters. Interesting changes are in channel.h and channel_internal_api.c, where I use some horrible macros to generate setter declarations and definitions to avoid having to write 3 different setter functions for each string field (set, build, and build_va). Wrapping stringfields that are inside a struct that is going to be opaque is kind of a pain since we have to pass the field name to the stringfield macros. I'm not completely convinced that the macros are the way to go, but they do certainly use a lot less space. I'm also not sure the best way to handle doxygen comments for such things...just comment the macros for the declarations, or where we call the macros... Opinions?
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/app_saycountpl.c 350231 
>   /trunk/addons/chan_mobile.c 350231 
>   /trunk/addons/chan_ooh323.c 350231 
>   /trunk/apps/app_authenticate.c 350231 
>   /trunk/apps/app_chanspy.c 350231 
>   /trunk/apps/app_confbridge.c 350231 
>   /trunk/apps/app_dial.c 350231 
>   /trunk/apps/app_dictate.c 350231 
>   /trunk/apps/app_directory.c 350231 
>   /trunk/apps/app_disa.c 350231 
>   /trunk/apps/app_dumpchan.c 350231 
>   /trunk/apps/app_externalivr.c 350231 
>   /trunk/apps/app_followme.c 350231 
>   /trunk/apps/app_meetme.c 350231 
>   /trunk/apps/app_minivm.c 350231 
>   /trunk/apps/app_page.c 350231 
>   /trunk/apps/app_parkandannounce.c 350231 
>   /trunk/apps/app_playback.c 350231 
>   /trunk/apps/app_privacy.c 350231 
>   /trunk/apps/app_queue.c 350231 
>   /trunk/apps/app_readexten.c 350231 
>   /trunk/apps/app_record.c 350231 
>   /trunk/apps/app_rpt.c 350231 
>   /trunk/apps/app_sayunixtime.c 350231 
>   /trunk/apps/app_speech_utils.c 350231 
>   /trunk/apps/app_stack.c 350231 
>   /trunk/apps/app_talkdetect.c 350231 
>   /trunk/apps/app_userevent.c 350231 
>   /trunk/apps/app_voicemail.c 350231 
>   /trunk/channels/chan_agent.c 350231 
>   /trunk/channels/chan_alsa.c 350231 
>   /trunk/channels/chan_bridge.c 350231 
>   /trunk/channels/chan_console.c 350231 
>   /trunk/channels/chan_dahdi.c 350231 
>   /trunk/channels/chan_gtalk.c 350231 
>   /trunk/channels/chan_h323.c 350231 
>   /trunk/channels/chan_iax2.c 350231 
>   /trunk/channels/chan_jingle.c 350231 
>   /trunk/channels/chan_local.c 350231 
>   /trunk/channels/chan_mgcp.c 350231 
>   /trunk/channels/chan_misdn.c 350231 
>   /trunk/channels/chan_multicast_rtp.c 350231 
>   /trunk/channels/chan_nbs.c 350231 
>   /trunk/channels/chan_oss.c 350231 
>   /trunk/channels/chan_phone.c 350231 
>   /trunk/channels/chan_sip.c 350231 
>   /trunk/channels/chan_skinny.c 350231 
>   /trunk/channels/chan_unistim.c 350231 
>   /trunk/channels/chan_usbradio.c 350231 
>   /trunk/channels/sig_analog.c 350231 
>   /trunk/channels/sig_pri.c 350231 
>   /trunk/funcs/func_channel.c 350231 
>   /trunk/funcs/func_global.c 350231 
>   /trunk/include/asterisk/channel.h 350231 
>   /trunk/main/aoc.c 350231 
>   /trunk/main/app.c 350231 
>   /trunk/main/cdr.c 350231 
>   /trunk/main/cel.c 350231 
>   /trunk/main/channel.c 350231 
>   /trunk/main/channel_internal_api.c 350231 
>   /trunk/main/cli.c 350231 
>   /trunk/main/dial.c 350231 
>   /trunk/main/features.c 350231 
>   /trunk/main/file.c 350231 
>   /trunk/main/image.c 350231 
>   /trunk/main/manager.c 350231 
>   /trunk/main/pbx.c 350231 
>   /trunk/main/say.c 350231 
>   /trunk/pbx/pbx_realtime.c 350231 
>   /trunk/res/res_agi.c 350231 
>   /trunk/res/res_fax.c 350231 
>   /trunk/res/res_monitor.c 350231 
>   /trunk/res/res_musiconhold.c 350231 
>   /trunk/res/snmp/agent.c 350231 
> 
> Diff: https://reviewboard.asterisk.org/r/1661/diff
> 
> 
> Testing
> -------
> 
> Ran the testsuite.
> 
> 
> Thanks,
> 
> Terry
> 
>

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