[asterisk-dev] [Code Review] Create API for doing variable substitution with dynamic buffers

[Russell Bryant]

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



/trunk/apps/app_minivm.c
<http://reviewboard.digium.com/r/174/#comment1744>

    I think the return value should be const char *
    
    Also, maxlen should be size_t



/trunk/apps/app_minivm.c
<http://reviewboard.digium.com/r/174/#comment1745>

    return const char *, make maxlen size_t



/trunk/apps/app_voicemail.c
<http://reviewboard.digium.com/r/174/#comment1746>

    maxlen -> size_t



/trunk/apps/app_voicemail.c
<http://reviewboard.digium.com/r/174/#comment1747>

    maxlen -> size_t
    
    Also, return const char *



/trunk/funcs/func_aes.c
<http://reviewboard.digium.com/r/174/#comment1749>

    This is an unrelated behavior change to func_aes.



/trunk/include/asterisk/ast_expr.h
<http://reviewboard.digium.com/r/174/#comment1735>

    Here is another new API call that needs documentation.



/trunk/include/asterisk/astobj2.h
<http://reviewboard.digium.com/r/174/#comment1736>

    Can you please merge all of these (and other) changes unrelated changes into trunk so that the diff is easier to review?



/trunk/include/asterisk/pbx.h
<http://reviewboard.digium.com/r/174/#comment1738>

    ssize_t here, too



/trunk/include/asterisk/pbx.h
<http://reviewboard.digium.com/r/174/#comment1737>

    I would make hintsize a ssize_t.



/trunk/include/asterisk/pbx.h
<http://reviewboard.digium.com/r/174/#comment1739>

    Please add a \brief to this and the other new functions nearby.  Also, please document the return value.



/trunk/include/asterisk/pbx.h
<http://reviewboard.digium.com/r/174/#comment1740>

    This isn't a big deal, but doxygen also has another tag for documenting individual return values:
    
    \retval 0 success
    \retval non-zero failure



/trunk/include/asterisk/pbx.h
<http://reviewboard.digium.com/r/174/#comment1741>

    Make maxlen a ssize_t



/trunk/main/pbx.c
<http://reviewboard.digium.com/r/174/#comment1742>

    I think this should return a const char *, to avoid directly modifying the internal ast_str buffer.



/trunk/main/pbx.c
<http://reviewboard.digium.com/r/174/#comment1743>

    Indentation could be easily reduced here.


- Russell


On 2009-03-31 21:40:43, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/174/
> -----------------------------------------------------------
> 
> (Updated 2009-03-31 21:40:43)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> We currently have a variable substitution system that depends upon static buffers.  This has issues, both because it takes up 12k of stack each time through nested variable expressions, but also because in some newer cases, the 4k limit on the size of the buffer is no longer sufficient.  So this is a conversion that allows us to use dynamic string buffers in place of the old static buffers.
> 
> Additionally, note that this is a subset of the changes done, to make review of the changes easier.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_exec.c 185116 
>   /trunk/apps/app_macro.c 185116 
>   /trunk/apps/app_minivm.c 185116 
>   /trunk/apps/app_voicemail.c 185116 
>   /trunk/cdr/cdr_custom.c 185116 
>   /trunk/funcs/func_aes.c 185116 
>   /trunk/funcs/func_base64.c 185116 
>   /trunk/funcs/func_blacklist.c 185116 
>   /trunk/funcs/func_callerid.c 185116 
>   /trunk/funcs/func_curl.c 185116 
>   /trunk/funcs/func_cut.c 185116 
>   /trunk/funcs/func_db.c 185116 
>   /trunk/funcs/func_dialplan.c 185116 
>   /trunk/funcs/func_env.c 185116 
>   /trunk/funcs/func_extstate.c 185116 
>   /trunk/funcs/func_groupcount.c 185116 
>   /trunk/funcs/func_lock.c 185116 
>   /trunk/funcs/func_logic.c 185116 
>   /trunk/funcs/func_md5.c 185116 
>   /trunk/funcs/func_module.c 185116 
>   /trunk/funcs/func_rand.c 185116 
>   /trunk/funcs/func_sha1.c 185116 
>   /trunk/funcs/func_speex.c 185116 
>   /trunk/funcs/func_strings.c 185116 
>   /trunk/funcs/func_sysinfo.c 185116 
>   /trunk/funcs/func_timeout.c 185116 
>   /trunk/funcs/func_vmcount.c 185116 
>   /trunk/include/asterisk.h 185116 
>   /trunk/include/asterisk/ast_expr.h 185116 
>   /trunk/include/asterisk/astobj2.h 185116 
>   /trunk/include/asterisk/pbx.h 185116 
>   /trunk/include/asterisk/res_odbc.h 185116 
>   /trunk/include/asterisk/strings.h 185116 
>   /trunk/main/ast_expr2f.c 185116 
>   /trunk/main/manager.c 185116 
>   /trunk/main/pbx.c 185116 
>   /trunk/main/strings.c 185116 
>   /trunk/main/taskprocessor.c 185116 
>   /trunk/main/tdd.c 185116 
>   /trunk/res/res_agi.c 185116 
>   /trunk/res/res_config_curl.c 185116 
>   /trunk/res/res_odbc.c 185116 
>   /trunk/res/res_phoneprov.c 185116 
> 
> Diff: http://reviewboard.digium.com/r/174/diff
> 
> 
> Testing
> -------
> 
> Extensive testing has been done on the new core API substitution call, though limited testing has only been done on the multiple places in the code where the new API has replaced the old substitution.
> 
> 
> Thanks,
> 
> Tilghman
> 
>