[asterisk-dev] [Code Review] 4526: clang compiler warning: -Wunused-value -Wunused-variable -Wunused-const-variable

[Diederik de Groot]

> On March 26, 2015, 6:37 p.m., Diederik de Groot wrote:
> > /branches/13/funcs/func_env.c, line 728
> > <https://reviewboard.asterisk.org/r/4526/diff/1/?file=72920#file72920line728>
> >
> >     Note the double closing parens after the first sizeof, i actually finishes the ast_str_append_substr command and then continues with the second sizeof.
> >     
> >     Not a 100% sure if this is corrected in the right way, but the original doesn't look right either.
> >
> 
> Matt Jordan wrote:
>     I'd double check that the unit tests still pass with the change.
> 
> Diederik de Groot wrote:
>     Currently not running any tests, just making the changes for now. Sounds silly, but it is enough work as it is. Automated testing should spit out any issues later on i hope.
> 
> Matt Jordan wrote:
>     Well, that would mean someone else would have to go fix it. :-P
>     
>     It's really easy to run a single unit test to double check that the logic still works - particularly when there's already a unit test written for a particular module. To do that, you:
>     
>     * Configure Asterisk to enable dev mode: ./configure --enable-dev-mode
>     * Enable TEST_FRAMEWORK in menuselect's build options
>     * On the CLI, execute 'test execute category /funcs/func_env/'
>     
>     Granted, since Asterisk won't compile in dev mode with clang (as WARNINGs are promoted to ERRORs), you'll need to double check it compiled with gcc, but for things like this where stuff is questionable, it doesn't hurt to make sure that the existing tests pass locally before things get committed.
> 
> Diederik de Groot wrote:
>     Now that i am finished i can run a couple of these, i guess. Will give it a go, after my CPU cools down a bit ;-P

Done:
Test passes both with and without the patch, both under gcc compiled asterisk as well as clang compiled asterisk: Result PASS. 

SideNote1: All this work i have been doing was to make asterisk compilable clang, so it should be able to run these tests as well, right. With all the submitted patches in place it compiles and runs in dev-mode as well.

SideNote2: Because of the discovered error in this function i guess the test should / would have to be updated for this particular case to prevent future regressions.


- Diederik


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


On March 27, 2015, 12:09 p.m., Diederik de Groot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4526/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 12:09 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24917
>     https://issues.asterisk.org/jira/browse/ASTERISK-24917
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs
> 
> clang compiler warning:-Wunused-value -Wunused-variable -Wunused-const-variable
> 
> Changes:
> apps/app_queue.c: removed unused qpm_cmd_usage[], qum_cmd_usage[], qsmp_cmd_usage[]
> cel/cel_sqlite3_custom.c: removed unused name[] = "cel_sqlite3_custom"
> channels/chan_pjsip.c: removed unused desc[] = "PJSIP Channel"
> codecs/gsm/src/gsm_create.c: removed unused ident[] = "$Header$"
> funcs/func_env.c:729: Fixed ast_str_append_substr. Needs to be reviewed by code owner.
> main/config.c: inline functions have to be static for clang to grok them
> main/editline/np/strlcat.c: removed unused rcsid = "$OpenBSD: strlcat.c,v 1.2 1999/06/17 16:28:58 millert Exp $"
> main/editline/np/strlcpy.c: removed unused rcsid = "$OpenBSD: strlcpy.c,v 1.4 1999/05/01 18:56:41 millert Exp $"
> main/features.c: removed unused channel_app_data_datastore struct
> main/security_events.c: removed unused TIMESTAMP_STR_LEN
> utils/conf2ael.c: removed unused cfextension_states
> utils/extconf.c: removed unused cfextension_states
> 
> 
> Diffs
> -----
> 
>   /branches/13/utils/extconf.c 433444 
>   /branches/13/utils/conf2ael.c 433444 
>   /branches/13/main/security_events.c 433444 
>   /branches/13/main/features.c 433444 
>   /branches/13/main/editline/np/strlcpy.c 433444 
>   /branches/13/main/editline/np/strlcat.c 433444 
>   /branches/13/main/config.c 433470 
>   /branches/13/funcs/func_env.c 433444 
>   /branches/13/codecs/gsm/src/gsm_create.c 433444 
>   /branches/13/channels/chan_pjsip.c 433444 
>   /branches/13/cel/cel_sqlite3_custom.c 433444 
>   /branches/13/apps/app_queue.c 433444 
> 
> Diff: https://reviewboard.asterisk.org/r/4526/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150327/c861c830/attachment-0001.html>