[asterisk-dev] [Code Review]: avoid many cppcheck (#2)

[junky]

> On March 1, 2012, 7:12 a.m., wdoekes wrote:
> > trunk/res/res_musiconhold.c, lines 1104-1105
> > <https://reviewboard.asterisk.org/r/1743/diff/2/?file=25194#file25194line1104>
> >
> >     Move this one up a line, it belongs with the ast_free, not with the getcwd.
> 
> junky wrote:
>     i dont think so.
>     there was no { } before, so it means 
>     class->total_files = 0;
>     was called just once, after the for loop.
>     It doesnt make sense to call that initialisation many times, no?
>     
>     I added braces though.
> 
> wdoekes wrote:
>     I did not say you should move the total_files=0 to *inside* the for-loop. I just said it should move up one line.
>     
>     Why? Because when I was looking for what happens if we break early from there and re-enter the function later: I saw free's, but I didn't see any nullification to prevent the free's from being done a second time. Turns out, it was there, but was leaning against the getcwd-check.
>     
>     It has nothing to do with braces, just with logical ordering. I wanted:
>     
>     BLOCK_THAT_FREES_STUFF_AND_SETS_IT_TO_ZERO
>     ENTER
>     BLOCK_THAT_DOES_GETCWD
>     
>     and not 
>     
>     BLOCK_THAT_FREES_STUFF
>     ENTER
>     BLOCK_THAT_SETS_SOMETHING_TO_ZERO_AND_DOES_GETCWD

ha, changed.


> On March 1, 2012, 7:12 a.m., wdoekes wrote:
> > trunk/channels/chan_unistim.c, lines 3158-3161
> > <https://reviewboard.asterisk.org/r/1743/diff/2/?file=25176#file25176line3158>
> >
> >     Wrong: OpenHistory takes care of most of the fclose's.
> >     
> >     (1) Change OpenHistory to set *f = NULL after fclose and here do an if(f){fclose(f);}
> >     
> >     or
> >     
> >     (2) Have OpenHistory fclose(*f) if count==0.
> >     
> >     Logically, I'd prefer the first. But the second will reduce the amount of extra if's you'll have to write. Better go with the last one.
> 
> junky wrote:
>     i would tend to agree, but not cppcheck.
>     Do you see any trouble to have those fclose() in there?
> 
> wdoekes wrote:
>     Yes, I see trouble. Thou shall not fclose a FILE pointer twice. And in all cases of errors, OpenHistory already closes it. The only missing fclose is when there is no error, but count is 0.

now, in case there's no error, but count is 0, the fclose() is done.


- junky


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


On March 1, 2012, 5:13 p.m., junky wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1743/
> -----------------------------------------------------------
> 
> (Updated March 1, 2012, 5:13 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch is to prevent many more cppcheck warnings.
> 
> 
> Diffs
> -----
> 
>   trunk/apps/app_minivm.c 357660 
>   trunk/apps/app_osplookup.c 357660 
>   trunk/cdr/cdr_pgsql.c 357660 
>   trunk/cdr/cdr_sqlite3_custom.c 357660 
>   trunk/channels/chan_alsa.c 357660 
>   trunk/channels/chan_dahdi.c 357660 
>   trunk/channels/chan_gtalk.c 357660 
>   trunk/channels/chan_h323.c 357660 
>   trunk/channels/chan_sip.c 357660 
>   trunk/channels/chan_unistim.c 357660 
>   trunk/channels/vcodecs.c 357660 
>   trunk/codecs/codec_dahdi.c 357660 
>   trunk/codecs/codec_g726.c 357660 
>   trunk/codecs/codec_resample.c 357660 
>   trunk/formats/format_h264.c 357660 
>   trunk/funcs/func_callerid.c 357660 
>   trunk/funcs/func_devstate.c 357660 
>   trunk/funcs/func_env.c 357660 
>   trunk/funcs/func_groupcount.c 357660 
>   trunk/funcs/func_odbc.c 357660 
>   trunk/main/ast_expr2.fl 357660 
>   trunk/main/ast_expr2f.c 357660 
>   trunk/main/asterisk.c 357660 
>   trunk/main/data.c 357660 
>   trunk/res/res_config_ldap.c 357660 
>   trunk/res/res_config_sqlite3.c 357660 
>   trunk/res/res_corosync.c 357660 
>   trunk/res/res_format_attr_celt.c 357660 
>   trunk/res/res_format_attr_silk.c 357660 
>   trunk/res/res_http_post.c 357660 
>   trunk/res/res_jabber.c 357660 
>   trunk/res/res_musiconhold.c 357660 
>   trunk/res/res_odbc.c 357660 
>   trunk/res/res_phoneprov.c 357660 
> 
> Diff: https://reviewboard.asterisk.org/r/1743/diff
> 
> 
> Testing
> -------
> 
> used cppcheck 1.52
> it compiles ;)
> 
> 
> Thanks,
> 
> junky
> 
>

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