[asterisk-dev] [Code Review] avoid cppcheck warnings

[wdoekes]

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


A couple of more points.


trunk/apps/app_osplookup.c
<https://reviewboard.asterisk.org/r/1651/#comment9620>

    /me points some more at this code.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1651/#comment9624>

    You *are* changing functionality:
    
    if MONITOR_EXEC == NULL but MONITOR_EXEC_ARGS isn't, then.
    
    Previously: which == qe->chan
    Now: which == peer
    
    Are you sure you want to do that? And if you are, then you should fix the inline if below, because then monexec is not NULL ever anymore.
    
    (And if you're not, one may need to reconsider the "if (!ast_strlen_zero(monexec)) {" below.. maybe it needs to be "if (!ast_strlen_zero(monexec) || !ast_strlen_zero(monargs)) {".)



trunk/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/1651/#comment9623>

    You're changing an error message here:
    
    in init_mailstream, we have:
    
            if (vms->mailstream == NIL) {
                    return -1;
            } else {
                    return 0;
            }
    
    NIL happens to be 0 (which should be checked instead, but that's a different matter).
    
    (The only other time init_mailstream returns non-zero is when vms_p is NULL, but it isn't.)
    
    So now the error is "Unable to open ..." instead of "IMAP mailstream is ..." when !vms_p->mailstream.
    
    The rest of the code is inconsistent about checking against !vms_p->mailstream or vms_p->mailstream == NIL. I don't expect you to fix that.
    
    You could combine this into a single if, removing the second check, but using the second error.
    
    (P.S. A similar pattern of not checking init_mailstream return and only !somevar->mailstream happens later on as well on line 7456.)



trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/1651/#comment9622>

    Thanks for adding this comment (and the other one above).



trunk/main/ast_expr2f.c
<https://reviewboard.asterisk.org/r/1651/#comment9621>

    Since you're touching this line, you may as well fix the indentation (braces, correct spacing around the for-arguments).


- wdoekes


On Jan. 10, 2012, 8:49 p.m., junky wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1651/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2012, 8:49 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> By running cppcheck 1.52, i realized there was many errors/warnings.
> 
> This patch fixes many of those.
> 
> 
> Diffs
> -----
> 
>   trunk/addons/chan_mobile.c 349670 
>   trunk/addons/chan_ooh323.c 349670 
>   trunk/apps/app_alarmreceiver.c 349670 
>   trunk/apps/app_chanspy.c 349670 
>   trunk/apps/app_disa.c 349670 
>   trunk/apps/app_minivm.c 349670 
>   trunk/apps/app_osplookup.c 349670 
>   trunk/apps/app_queue.c 349670 
>   trunk/apps/app_voicemail.c 349670 
>   trunk/channels/chan_dahdi.c 349670 
>   trunk/channels/chan_iax2.c 349670 
>   trunk/channels/chan_misdn.c 349670 
>   trunk/channels/chan_skinny.c 349670 
>   trunk/channels/chan_usbradio.c 349670 
>   trunk/formats/format_h263.c 349670 
>   trunk/funcs/func_env.c 349670 
>   trunk/funcs/func_odbc.c 349670 
>   trunk/funcs/func_strings.c 349670 
>   trunk/main/acl.c 349670 
>   trunk/main/ast_expr2.c 349670 
>   trunk/main/ast_expr2f.c 349670 
>   trunk/main/pbx.c 349670 
>   trunk/main/udptl.c 349670 
>   trunk/res/res_phoneprov.c 349670 
>   trunk/utils/astman.c 349670 
> 
> Diff: https://reviewboard.asterisk.org/r/1651/diff
> 
> 
> Testing
> -------
> 
> still compile fine.
> Shouldn't have any impact on the code execution.
> 
> 
> Thanks,
> 
> junky
> 
>

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