Re: [asterisk-dev] [Code Review] 4613: clang compiler warning: clang compilation
On April 20, 2015, 1:40 p.m., Matt Jordan wrote: Over the past week, Asterisk has moved to Git. The migration was announced and discussed on the asterisk-dev list. We are now using Gerrit for code reviews (gerrit.asterisk.org). You can find instructions for using Gerrit on the Asterisk wiki: https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage Information about the Git servers and other policies can be found here: https://wiki.asterisk.org/wiki/display/AST/Git+Usage While it's a bit of a pain to move the patch over, in the long run, this should make managing the clang fixup patches much easier. Diederik de Groot wrote: Hi Matt, Thanks for the heads up. Will make the move to Git + Gerrit. From what i read on the mailinglist posts i though you were still experimenting. I guess I missed the final move announcement :-) BTW: This is the last series of small patches that would be required to get clang compilation finished. So we are almost there. Moved to git + gerrit https://gerrit.asterisk.org/#/c/157 https://gerrit.asterisk.org/#/c/158 https://gerrit.asterisk.org/#/c/159 https://gerrit.asterisk.org/#/c/160 Closing this review - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4613/#review15197 --- On April 20, 2015, 12:38 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4613/ --- (Updated April 20, 2015, 12:38 p.m.) Review request for Asterisk Developers, Matt Jordan, Mark Michelson, and rmudgett. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang compilation. three warnings have been suppressed (in Makefile.rules), which were deemed unsuitable for asterisk: - unused-value - parentheses-equality - unused-command-line-argument changes can/could be ported back to asterisk-11 if required. Diffs - /branches/13/res/res_security_log.c 434725 /branches/13/makeopts.in 434725 /branches/13/main/security_events.c 434725 /branches/13/include/asterisk/utils.h 434725 /branches/13/include/asterisk/autoconfig.h.in 434725 /branches/13/contrib/scripts/clang-scan-build PRE-CREATION /branches/13/configure.ac 434725 /branches/13/configure UNKNOWN /branches/13/channels/chan_skinny.c 434725 /branches/13/autoconf/ast_check_strsep_array_bounds.m4 PRE-CREATION /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION /branches/13/Makefile.rules 434725 Diff: https://reviewboard.asterisk.org/r/4613/diff/ Testing --- compiles cleanly test execute all 455 Test(s) Executed 454 Passed 1 Failed FAIL test_message_queue_handler_nom /main/message/ 21200ms = START /main/message/ - test_message_queue_handler_nominal [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message END/main/message/ - test_message_queue_handler_nominal Time: 21241ms Result: FAIL = 1 Test(s) Executed 0 Passed 1 Failed == Message handler 'testmsg' registered. -- Executing [s@default:1] wait(Message/ast_msg_queue, 1) -- Executing [s@default:1] answer(Message/ast_msg_queue, ) -- Digit timeout set to 5.000 -- Response timeout set to 10.000 -- Executing [s@default:1] background(Message/ast_msg_queue, demo-congrats) [Apr 18 15:22:50] ERROR[13650][C-]: channel.c:5423 int set_format(struct ast_channel *, struct ast_format_cap *, const int): Unable to set format because channel Message/ast_msg_queue supports no formats [Apr 18 15:22:50] WARNING[13650][C-]: file.c:1100 int ast_streamfile(struct ast_channel *, const char *, const char *): Unable to open demo-congrats (format (none)): Function not implemented [Apr 18 15:22:50] WARNING[13650][C-]: pbx.c:11319 int pbx_builtin_background(struct ast_channel *, const char *): ast_streamfile failed on Message/ast_msg_queue for demo-congrats Repeated Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4613: clang compiler warning: clang compilation
On April 20, 2015, 1:40 p.m., Matt Jordan wrote: Over the past week, Asterisk has moved to Git. The migration was announced and discussed on the asterisk-dev list. We are now using Gerrit for code reviews (gerrit.asterisk.org). You can find instructions for using Gerrit on the Asterisk wiki: https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage Information about the Git servers and other policies can be found here: https://wiki.asterisk.org/wiki/display/AST/Git+Usage While it's a bit of a pain to move the patch over, in the long run, this should make managing the clang fixup patches much easier. Hi Matt, Thanks for the heads up. Will make the move to Git + Gerrit. From what i read on the mailinglist posts i though you were still experimenting. I guess I missed the final move announcement :-) BTW: This is the last series of small patches that would be required to get clang compilation finished. So we are almost there. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4613/#review15197 --- On April 20, 2015, 12:38 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4613/ --- (Updated April 20, 2015, 12:38 p.m.) Review request for Asterisk Developers, Matt Jordan, Mark Michelson, and rmudgett. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang compilation. three warnings have been suppressed (in Makefile.rules), which were deemed unsuitable for asterisk: - unused-value - parentheses-equality - unused-command-line-argument changes can/could be ported back to asterisk-11 if required. Diffs - /branches/13/res/res_security_log.c 434725 /branches/13/makeopts.in 434725 /branches/13/main/security_events.c 434725 /branches/13/include/asterisk/utils.h 434725 /branches/13/include/asterisk/autoconfig.h.in 434725 /branches/13/contrib/scripts/clang-scan-build PRE-CREATION /branches/13/configure.ac 434725 /branches/13/configure UNKNOWN /branches/13/channels/chan_skinny.c 434725 /branches/13/autoconf/ast_check_strsep_array_bounds.m4 PRE-CREATION /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION /branches/13/Makefile.rules 434725 Diff: https://reviewboard.asterisk.org/r/4613/diff/ Testing --- compiles cleanly test execute all 455 Test(s) Executed 454 Passed 1 Failed FAIL test_message_queue_handler_nom /main/message/ 21200ms = START /main/message/ - test_message_queue_handler_nominal [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message END/main/message/ - test_message_queue_handler_nominal Time: 21241ms Result: FAIL = 1 Test(s) Executed 0 Passed 1 Failed == Message handler 'testmsg' registered. -- Executing [s@default:1] wait(Message/ast_msg_queue, 1) -- Executing [s@default:1] answer(Message/ast_msg_queue, ) -- Digit timeout set to 5.000 -- Response timeout set to 10.000 -- Executing [s@default:1] background(Message/ast_msg_queue, demo-congrats) [Apr 18 15:22:50] ERROR[13650][C-]: channel.c:5423 int set_format(struct ast_channel *, struct ast_format_cap *, const int): Unable to set format because channel Message/ast_msg_queue supports no formats [Apr 18 15:22:50] WARNING[13650][C-]: file.c:1100 int ast_streamfile(struct ast_channel *, const char *, const char *): Unable to open demo-congrats (format (none)): Function not implemented [Apr 18 15:22:50] WARNING[13650][C-]: pbx.c:11319 int pbx_builtin_background(struct ast_channel *, const char *): ast_streamfile failed on Message/ast_msg_queue for demo-congrats Repeated Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4613: clang compiler warning: clang compilation
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4613/ --- (Updated April 20, 2015, 3:05 p.m.) Status -- This change has been discarded. Review request for Asterisk Developers, Matt Jordan, Mark Michelson, and rmudgett. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang compilation. three warnings have been suppressed (in Makefile.rules), which were deemed unsuitable for asterisk: - unused-value - parentheses-equality - unused-command-line-argument changes can/could be ported back to asterisk-11 if required. Diffs - /branches/13/res/res_security_log.c 434725 /branches/13/makeopts.in 434725 /branches/13/main/security_events.c 434725 /branches/13/include/asterisk/utils.h 434725 /branches/13/include/asterisk/autoconfig.h.in 434725 /branches/13/contrib/scripts/clang-scan-build PRE-CREATION /branches/13/configure.ac 434725 /branches/13/configure UNKNOWN /branches/13/channels/chan_skinny.c 434725 /branches/13/autoconf/ast_check_strsep_array_bounds.m4 PRE-CREATION /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION /branches/13/Makefile.rules 434725 Diff: https://reviewboard.asterisk.org/r/4613/diff/ Testing --- compiles cleanly test execute all 455 Test(s) Executed 454 Passed 1 Failed FAIL test_message_queue_handler_nom /main/message/ 21200ms = START /main/message/ - test_message_queue_handler_nominal [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message END/main/message/ - test_message_queue_handler_nominal Time: 21241ms Result: FAIL = 1 Test(s) Executed 0 Passed 1 Failed == Message handler 'testmsg' registered. -- Executing [s@default:1] wait(Message/ast_msg_queue, 1) -- Executing [s@default:1] answer(Message/ast_msg_queue, ) -- Digit timeout set to 5.000 -- Response timeout set to 10.000 -- Executing [s@default:1] background(Message/ast_msg_queue, demo-congrats) [Apr 18 15:22:50] ERROR[13650][C-]: channel.c:5423 int set_format(struct ast_channel *, struct ast_format_cap *, const int): Unable to set format because channel Message/ast_msg_queue supports no formats [Apr 18 15:22:50] WARNING[13650][C-]: file.c:1100 int ast_streamfile(struct ast_channel *, const char *, const char *): Unable to open demo-congrats (format (none)): Function not implemented [Apr 18 15:22:50] WARNING[13650][C-]: pbx.c:11319 int pbx_builtin_background(struct ast_channel *, const char *): ast_streamfile failed on Message/ast_msg_queue for demo-congrats Repeated Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4613: clang compiler warning: clang compilation
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4613/ --- (Updated April 20, 2015, 12:38 p.m.) Review request for Asterisk Developers, Matt Jordan, Mark Michelson, and rmudgett. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang compilation. three warnings have been suppressed (in Makefile.rules), which were deemed unsuitable for asterisk: - unused-value - parentheses-equality - unused-command-line-argument changes can/could be ported back to asterisk-11 if required. Diffs - /branches/13/res/res_security_log.c 434725 /branches/13/makeopts.in 434725 /branches/13/main/security_events.c 434725 /branches/13/include/asterisk/utils.h 434725 /branches/13/include/asterisk/autoconfig.h.in 434725 /branches/13/contrib/scripts/clang-scan-build PRE-CREATION /branches/13/configure.ac 434725 /branches/13/configure UNKNOWN /branches/13/channels/chan_skinny.c 434725 /branches/13/autoconf/ast_check_strsep_array_bounds.m4 PRE-CREATION /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION /branches/13/Makefile.rules 434725 Diff: https://reviewboard.asterisk.org/r/4613/diff/ Testing --- compiles cleanly test execute all 455 Test(s) Executed 454 Passed 1 Failed FAIL test_message_queue_handler_nom /main/message/ 21200ms = START /main/message/ - test_message_queue_handler_nominal [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message END/main/message/ - test_message_queue_handler_nominal Time: 21241ms Result: FAIL = 1 Test(s) Executed 0 Passed 1 Failed == Message handler 'testmsg' registered. -- Executing [s@default:1] wait(Message/ast_msg_queue, 1) -- Executing [s@default:1] answer(Message/ast_msg_queue, ) -- Digit timeout set to 5.000 -- Response timeout set to 10.000 -- Executing [s@default:1] background(Message/ast_msg_queue, demo-congrats) [Apr 18 15:22:50] ERROR[13650][C-]: channel.c:5423 int set_format(struct ast_channel *, struct ast_format_cap *, const int): Unable to set format because channel Message/ast_msg_queue supports no formats [Apr 18 15:22:50] WARNING[13650][C-]: file.c:1100 int ast_streamfile(struct ast_channel *, const char *, const char *): Unable to open demo-congrats (format (none)): Function not implemented [Apr 18 15:22:50] WARNING[13650][C-]: pbx.c:11319 int pbx_builtin_background(struct ast_channel *, const char *): ast_streamfile failed on Message/ast_msg_queue for demo-congrats Repeated Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4547: clang compiler warning: braces-around-scalar-initializer
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4547/ --- (Updated April 18, 2015, 1:35 p.m.) Status -- This change has been discarded. 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:braces-around-scalar-initializer Diffs - /branches/13/res/res_pjsip_dtmf_info.c 433444 /branches/13/channels/sig_ss7.c 433444 /branches/13/channels/sig_pri.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/console_gui.c 433444 /branches/13/channels/chan_unistim.c 433444 /branches/13/channels/chan_skinny.c 433444 /branches/13/channels/chan_sip.c 433444 /branches/13/channels/chan_oss.c 433444 /branches/13/channels/chan_mgcp.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/channels/chan_dahdi.c 433444 /branches/13/channels/chan_console.c 433444 /branches/13/channels/chan_alsa.c 433444 /branches/13/apps/app_dictate.c 433444 Diff: https://reviewboard.asterisk.org/r/4547/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4613: clang compiler warning: clang compilation
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4613/ --- 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:clang compilation Diffs - /branches/13/res/res_security_log.c 434725 /branches/13/makeopts.in 434725 /branches/13/main/security_events.c 434725 /branches/13/include/asterisk/utils.h 434725 /branches/13/include/asterisk/autoconfig.h.in 434725 /branches/13/contrib/scripts/clang-scan-build PRE-CREATION /branches/13/configure.ac 434725 /branches/13/configure UNKNOWN /branches/13/channels/chan_skinny.c 434725 /branches/13/autoconf/ast_check_strsep_array_bounds.m4 PRE-CREATION /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION /branches/13/Makefile.rules 434725 Diff: https://reviewboard.asterisk.org/r/4613/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4546: clang compiler warning: -Warray-bounds
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/ --- (Updated April 18, 2015, 3:03 p.m.) Status -- This change has been discarded. 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:-Warray-bounds Diffs - /branches/13/configure.ac 433444 Diff: https://reviewboard.asterisk.org/r/4546/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4543: clang compiler warning: RAII-clang reentrancy / Updating variable from inside a _block
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/ --- (Updated April 18, 2015, 3:03 p.m.) Status -- This change has been discarded. Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- RAII-clang reentrancy / Updating variable from inside a _block To update the varname variable from inside the cleanup block it needs to be decorated with __block as in: __block vartype varname = ctor; Diffs - /branches/13/include/asterisk/utils.h 433444 /branches/13/contrib/scan-build PRE-CREATION /branches/13/configure.ac 433444 /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4543/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4550/ --- (Updated April 18, 2015, 1:26 p.m.) Status -- This change has been discarded. 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:--dev-mode and -Wparentheses-equality include/asterisk/linkedlists.h:450 == Got rid of the extraneous () in the comparison to NULL by negating the comparison include/asterisk/vector.h:261 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem) != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:467 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem)-format != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:492 = Because of the () where removed previously, they are now needed here. main/stasis_message_router.c:82 === Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem).message_type != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/stdtime/localtime.c:197/208 Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((sp)-wd[0] != SP_STACK_FLAG) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. more of the same for include/asterisk/dlinkedlists.h:422/431/469 main/astobj2_hash.c:768 - Maybe one of you has a better/nicer solution. Diffs - /branches/13/tests/test_linkedlists.c 433444 /branches/13/main/stdtime/localtime.c 433444 /branches/13/main/stasis_message_router.c 433444 /branches/13/main/format_cap.c 433444 /branches/13/main/astobj2_hash.c 433444 /branches/13/include/asterisk/vector.h 433444 /branches/13/include/asterisk/linkedlists.h 433444 /branches/13/include/asterisk/dlinkedlists.h 433444 Diff: https://reviewboard.asterisk.org/r/4550/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4613: clang compiler warning: clang compilation
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4613/ --- (Updated April 18, 2015, 3:26 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description (updated) --- clang compilation. three warnings have been suppressed (in Makefile.rules), which were deemed unsuitable for asterisk: - unused-value - parentheses-equality - unused-command-line-argument changes can/could be ported back to asterisk-11 if required. Diffs - /branches/13/res/res_security_log.c 434725 /branches/13/makeopts.in 434725 /branches/13/main/security_events.c 434725 /branches/13/include/asterisk/utils.h 434725 /branches/13/include/asterisk/autoconfig.h.in 434725 /branches/13/contrib/scripts/clang-scan-build PRE-CREATION /branches/13/configure.ac 434725 /branches/13/configure UNKNOWN /branches/13/channels/chan_skinny.c 434725 /branches/13/autoconf/ast_check_strsep_array_bounds.m4 PRE-CREATION /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION /branches/13/Makefile.rules 434725 Diff: https://reviewboard.asterisk.org/r/4613/diff/ Testing (updated) --- compiles cleanly test execute all 455 Test(s) Executed 454 Passed 1 Failed FAIL test_message_queue_handler_nom /main/message/ 21200ms = START /main/message/ - test_message_queue_handler_nominal [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message END/main/message/ - test_message_queue_handler_nominal Time: 21241ms Result: FAIL = 1 Test(s) Executed 0 Passed 1 Failed == Message handler 'testmsg' registered. -- Executing [s@default:1] wait(Message/ast_msg_queue, 1) -- Executing [s@default:1] answer(Message/ast_msg_queue, ) -- Digit timeout set to 5.000 -- Response timeout set to 10.000 -- Executing [s@default:1] background(Message/ast_msg_queue, demo-congrats) [Apr 18 15:22:50] ERROR[13650][C-]: channel.c:5423 int set_format(struct ast_channel *, struct ast_format_cap *, const int): Unable to set format because channel Message/ast_msg_queue supports no formats [Apr 18 15:22:50] WARNING[13650][C-]: file.c:1100 int ast_streamfile(struct ast_channel *, const char *, const char *): Unable to open demo-congrats (format (none)): Function not implemented [Apr 18 15:22:50] WARNING[13650][C-]: pbx.c:11319 int pbx_builtin_background(struct ast_channel *, const char *): ast_streamfile failed on Message/ast_msg_queue for demo-congrats Repeated Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 18, 2015, 1:29 p.m.) Status -- This change has been discarded. 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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 11, 2015, 10:26 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 434705 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. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
On March 31, 2015, 7:14 p.m., Mark Michelson wrote:
I don't understand the purpose of this warning. I tried searching for
details about this warning flag on the internet and came up empty, so I
can't find any documentation that explains what type of error this check is
supposed to help avoid.
It appears that the warning is intended to get rid of extra parentheses
where they are unnecessary. The problem is that I don't see anything wrong
with having them there, especially in macro definitions.
Diederik de Groot wrote:
Ok you know that to prevent an unintendet assignment where a comparisson
was indended compilers request you to write
/* comparisson */
if (x == 1) {
}
but
/* assignment = most modern compilers will complain*/
if (x = 1) { /* = warning raised*/
}
so you are forced to write
if ((x = 1)) {
}
instead, to guarantee that you meant to assign a value here.
But most compilers will not complain if you write the first comparison
between double parantheses
if ((x == 1)) {
}
Which is perfectly legal, as you pointed out.
But a maintainer of the code might later be in doubt as to what you mean.
Was an assignment was not intended by the original writer (non-obvious).
clang can be made complain about this (-Wparantheses-equalty or -Wall
-Werror) to make sure this last one is not allowed.
So this can be considered the reverse of the first warning, where an
assignment requires double parantheses.
rmudgett wrote:
I think this warning is a backlash from the warning about putting
assignments inside tests that you suppress by adding parentheses.
if ((a = b))
At any rate this is a very bad warning and should not be used.
The parentheses in macros are to prevent unexpected precedence operator
bindings:
#define BAD_MACRO(a,b) a + b
if (BAD_MACRO(a, b) * 3 != (a + b) * 3) printf(ERROR unexpected
result\n);
Diederik de Groot wrote:
@rmudgett: I agreed, that's why i was asking in my initial description,
if anybody knew of a better way to solve both the macro issue but still
satisfy the parantheses warning. Without having to suppress it. Something
ugly to fix the macro expansion is to use a double negating comparison or
maybe even an inline comparison return TRUE/FALSE. Both of which did not
really strike me as particularly nice.
I can live with suppressing this warning, but it would be nice if
somebody knew of a nice way to have it both ways.
rmudgett wrote:
Please discard this review with predjudice. I see no benefit to this
-Wparentheses-equality option.
Diederik de Groot wrote:
WOuld have been nice if this Warning would not react to results coming
from macro's (so only plain code), but alas. I guess the AST does not have
that last little detail.
I guess we should drop this one. Maybe someone will come up with a nice
solution in the future. There is a open issue on the clang bug report site
about this exact warning (currently without a nice solution).
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
---
On April 1, 2015, 3:33 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/
---
(Updated April 1, 2015, 3:33 a.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:--dev-mode and -Wparentheses-equality
include/asterisk/linkedlists.h:450
==
Got rid of the extraneous () in the comparison to NULL by negating the
comparison
include/asterisk/vector.h:261
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison !((elem)
!= (value)) which would keep everything encapsulated, but does result in a
double negative, which is ugly as well.
main/format_cap.c:467
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 9, 2015, 7:47 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 434469 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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Compiles without warning Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4547: clang compiler warning: braces-around-scalar-initializer
On March 28, 2015, 3:59 p.m., Matt Jordan wrote:
/branches/13/channels/chan_iax2.c, lines 7674-7676
https://reviewboard.asterisk.org/r/4547/diff/1/?file=73110#file73110line7674
I really dislike this warning.
In C, it has always been perfectly valid to initialize all members of a
struct using { 0 } as a universal zero-initializer. This nomenclature
actually makes it less readable, as it makes it look like we only wanted to
initialize the .frametype member, and ignored the rest. That may not be the
actual effect, but the syntax here is not clearer by any stretch.
:-(
Interestingly, when compiling with clang 3.4, I don't get this warning
issued. What version are you compiling with? And what do you think about
simply ignoring this warning?
Diederik de Groot wrote:
Using struct ast_frame f = { 0, }; is perfectly fine and does not raise
an error
However: struct ast_frame f = { AST_FRAME_CONTROL, {
AST_CONTROL_CONGESTION } }; however does raise this warning, which does make
a little more sense.
I updated all of these (i hope i got them all) so that they all use the
same syntax. In case someone ones to update/change/extend one of them.
FYI: There is one major benefit in using the named variety, namely
allowing you to change the order of the fields in the struct without
consequence. For example the ast_frame struct could be optimized a little by
reordering the fields to improve packing, as in:
struct ast_frame {
/*! Kind of frame */
enum ast_frame_type frametype;
/*! Length of data */
int datalen;
/*! Subclass, frame dependent */
struct ast_frame_subclass subclass;
/*! Number of samples in this frame */
int samples;
/*! Was the data malloc'd? i.e. should we free it when we
discard the frame? */
int mallocd;
/*! The number of bytes allocated for a malloc'd frame header */
size_t mallocd_hdr_len;
/*! How many bytes exist _before_ data that can be used if
needed */
int offset;
/*! Misc. frame flags */
unsigned int flags;
/*! Optional source of frame for debugging */
const char *src;
/*! Pointer to actual data */
union { void *ptr; uint32_t uint32; char pad[8]; } data;
/*! Global delivery time */
struct timeval delivery;
/*! For placing in a linked list */
AST_LIST_ENTRY(ast_frame) frame_list;
/*! Timestamp in milliseconds */
long ts;
/*! Length in milliseconds */
long len;
/*! Sequence number */
int seqno;
};
would get rid of some of the padding (on x86_64).
Note: this warning might have occured after playing with the struct
layout of ast_frame to be honest (Need to recheck this). If so this change
should be considered an enhancement.
Note2: I compile using clang-3.6
Any update on this ?
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4547/#review14941
---
On March 27, 2015, 7:34 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4547/
---
(Updated March 27, 2015, 7:34 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:braces-around-scalar-initializer
Diffs
-
/branches/13/res/res_pjsip_dtmf_info.c 433444
/branches/13/channels/sig_ss7.c 433444
/branches/13/channels/sig_pri.c 433444
/branches/13/channels/pjsip/dialplan_functions.c 433444
/branches/13/channels/console_gui.c 433444
/branches/13/channels/chan_unistim.c 433444
/branches/13/channels/chan_skinny.c 433444
/branches/13/channels/chan_sip.c 433444
/branches/13/channels/chan_oss.c 433444
/branches/13/channels/chan_mgcp.c 433444
/branches/13/channels/chan_iax2.c 433444
/branches/13/channels/chan_dahdi.c 433444
/branches/13/channels/chan_console.c 433444
/branches/13/channels/chan_alsa.c 433444
/branches/13/apps/app_dictate.c 433444
Diff: https://reviewboard.asterisk.org/r/4547/diff/
Testing
---
Thanks,
Diederik de
Re: [asterisk-dev] [Code Review] 4543: clang compiler warning: RAII-clang reentrancy / Updating variable from inside a _block
On March 29, 2015, 5:01 a.m., Diederik de Groot wrote: /branches/13/configure.ac, line 389 https://reviewboard.asterisk.org/r/4543/diff/6/?file=73280#file73280line389 move the raii compiler checks to their own m4 file and called the checking function a little earlier in the configure process Is this ok, or would you rather keep everything together in configure.ac. I think splitting out some stuff makes it a little more easy to maintain / test - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/#review14954 --- On March 29, 2015, 5:15 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/ --- (Updated March 29, 2015, 5:15 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- RAII-clang reentrancy / Updating variable from inside a _block To update the varname variable from inside the cleanup block it needs to be decorated with __block as in: __block vartype varname = ctor; Diffs - /branches/13/include/asterisk/utils.h 433444 /branches/13/contrib/scan-build PRE-CREATION /branches/13/configure.ac 433444 /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4543/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4541: clang compiler warning: -Wpointer-bool-conversion
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated April 8, 2015, 6:42 a.m.)
Status
--
This change has been marked as submitted.
Review request for Asterisk Developers.
Changes
---
Committed in revision 434285
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:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4541: clang compiler warning: -Wpointer-bool-conversion
On April 7, 2015, 3:05 a.m., rmudgett wrote:
/branches/13/apps/app_minivm.c, line 1842
https://reviewboard.asterisk.org/r/4541/diff/3/?file=73407#file73407line1842
Missing the !
if (!ast_strlen_zero())
Diederik de Groot wrote:
Thanks again for checking my stuff, and sorry for the mistakes.
Mark Michelson wrote:
Sorry for telling you wrong to begin with.
That's what you get when copying from other people without discriminating the
input... Should have learned that in school at some point :-)
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/#review15081
---
On April 7, 2015, 4:23 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated April 7, 2015, 4:23 a.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:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term'
will always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 8, 2015, 1:26 a.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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing (updated) --- Compiles without warning Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4541: clang compiler warning: -Wpointer-bool-conversion
On April 7, 2015, 9:32 p.m., rmudgett wrote:
/branches/13/apps/app_minivm.c, line 1872
https://reviewboard.asterisk.org/r/4541/diff/4/?file=73676#file73676line1872
Might as well fix the comment typo while changing the line anyway.
Rest - Reset
Another One Done.. Just a few left.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/#review15106
---
On April 8, 2015, 1:18 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated April 8, 2015, 1:18 a.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:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term'
will always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4541: clang compiler warning: -Wpointer-bool-conversion
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated April 8, 2015, 1:18 a.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:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs (updated)
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
On April 1, 2015, 3:16 a.m., rmudgett wrote: /branches/13/funcs/func_curl.c, lines 174-175 https://reviewboard.asterisk.org/r/4533/diff/7/?file=73369#file73369line174 Try defining this as: #define CURLOPT_SPECIAL_HASHCOMPAT ((CURLoption) -500) This should shut-up the compiler without changing the binary value. Diederik de Groot wrote: CurlOption is a positive enum though. So the warning about it not being allowed to be negative remains, won't it ? BTW: I made a typo while changing this one I was meaning to set it to: #define CURLOPT_SPECIAL_HASHCOMPAT CURLOPT_LASTENTRY Which is a legal value. Not really happy with this though. Setting it to #define CURLOPT_SPECIAL_HASHCOMPAT ((CURLoption) 500) Might be an option, not sure if this is checked though. This would require a little further source investigation. rmudgett wrote: The cast should shut the compiler up as you are telling the compiler to reinterpret the -500 to be a member of the enum. If not then it's another of those compilers that tenatiously guards their stupidity. Reinterpreting a negative number as an unsigned value results in a very large number on two's complement machines. Are there any modern machines that don't use two's complement? Using CURLOPT_LASTENTRY should also work as a last resort since it has the down side of library updates adding new enum values and changing the last entry value. rmudgett wrote: This is the last issue I have with this patch. I'd say ship it if clang is happy with my initial cast suggestion. Will do ! clang is just fine with it. does go against my grain a little, especially because we end up out of bounds of the CURLOPT_LASTENTRY, which could get checked at some time in the curl source code (if not already). Making the change though. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/#review15003 --- On April 1, 2015, 5:22 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 1, 2015, 5:22 a.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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 8, 2015, 1:25 a.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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4540: clang compiler warning: -Wformat
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4540/ --- (Updated April 6, 2015, 12:52 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 434087 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:-Wformat Issue: framehook.c:141:16: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat] i-version, AST_FRAMEHOOK_INTERFACE_VERSION); ^~~ /data/development/asterisk/asterisk-13-branch/include/asterisk/framehook.h:227:41: note: expanded from macro 'AST_FRAMEHOOK_INTERFACE_VERSION' #define AST_FRAMEHOOK_INTERFACE_VERSION 4 Changed format: to use %i instead of %hu Diffs - /branches/13/main/framehook.c 433444 Diff: https://reviewboard.asterisk.org/r/4540/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4541: clang compiler warning: -Wpointer-bool-conversion
On April 7, 2015, 3:05 a.m., rmudgett wrote:
/branches/13/apps/app_minivm.c, line 1842
https://reviewboard.asterisk.org/r/4541/diff/3/?file=73407#file73407line1842
Missing the !
if (!ast_strlen_zero())
Thanks again for checking my stuff, and sorry for the mistakes.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/#review15081
---
On April 7, 2015, 4:23 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated April 7, 2015, 4:23 a.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:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term'
will always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4551: clang compiler warning: -Wnon-literal-null-conversion
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4551/ --- (Updated April 6, 2015, 8:59 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 434187 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:-Wnon-literal-null-conversion Diffs - /branches/13/res/stasis_recording/stored.c 433444 /branches/13/channels/chan_skinny.c 433444 /branches/13/channels/chan_sip.c 433444 Diff: https://reviewboard.asterisk.org/r/4551/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4552: clang compiler warning: -Wsometimes-uninitialized
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4552/ --- (Updated April 6, 2015, 9:09 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 434190 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:-Wsometimes-uninitialized Diffs - /branches/13/funcs/func_math.c 433444 Diff: https://reviewboard.asterisk.org/r/4552/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 11:24 p.m.) Review request for Asterisk Developers. Changes --- Clarify Purpose of this Review entry 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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing (updated) --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 11:36 p.m., rmudgett wrote: This is still a nuisance warning that doesn't add much value for the effort. Diederik de Groot wrote: We can drop it no problem. I still think it could be useful in detecting _ref/_unref issues, alas it would quite a bit of work but could be beneficial. If there is consensus on the asterisk-developer side, i will make the required Makefile change to suppress this waning. I would like to make sure that you agree to drop the -Wunused-value warning, before i do so. So please report back. By the way did you check out this 'Posted 5 days, 21 hours ago (March 29, 2015, 1:49 a.m.)' entry on this page. I tried to explain how this warning could be benifial in finding these used after ..._unref issues. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15049 --- On April 3, 2015, 11:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 3, 2015, 11:24 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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2009-2013 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2009 This change causes bugs. It is supposed to return peer because it increased the ref. Stupid me. If should actually also check if ao2_ref did not return an error (-1). On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 11131-11136 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line11131 This is another case of dealing with the scheduler. We are releasing the ref we just tried to give the scheduler that failed to be added. made it a conscious choice choice to discard the pointer using (void)peer_unref(peer); Maybe time to update Scheduler :-) On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2021-2025 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2021 This change is another bug. It is supposed to return the found user. Guess i should not be doing any of this stuff at 3 Am i guess. Difficult to make a point, whilst making mistakes like these. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- On April 4, 2015, 1:15 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 a.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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 a.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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs (updated) - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 4, 2015, 12:40 a.m., rmudgett wrote: /branches/13/channels/chan_iax2.c, lines 2009-2013 https://reviewboard.asterisk.org/r/4554/diff/2/?file=73276#file73276line2009 This change causes bugs. It is supposed to return peer because it increased the ref. Diederik de Groot wrote: Stupid me. If should actually also check if ao2_ref did not return an error (-1). rmudgett wrote: The return value of ao2_ref is rarely used. When it is it is to get the number of refs the object had before the call. The -1 error return value is practically useless. That value means a programming error because the object is invalid and you get bad magic number messages logged, assertion failures, and very likely crashes. Ok, point taken. So that would mean that ao2_ref will be the biggest cause of -Wunused-value warning, even if we changed everything else, because it's return is rarely used :-) I still think it is valid to NULL a pointer that might/might not vanish in the future (as with ao2_ref(xx, -1)). Same goes for example for ast_free, which does not null the pointer afterwards (or did i overlook something ?. It makes NULL pointer checks after such a free or unref a little bit pointless. I know everything is working and asterisk-11 and asterisk-13 are pretty stable as far as i can tell, so not looking for extra work for any of us, just want to finish this point and move on. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15053 --- On April 4, 2015, 1:15 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 a.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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 3, 2015, 11:36 p.m., rmudgett wrote: This is still a nuisance warning that doesn't add much value for the effort. Diederik de Groot wrote: We can drop it no problem. I still think it could be useful in detecting _ref/_unref issues, alas it would quite a bit of work but could be beneficial. If there is consensus on the asterisk-developer side, i will make the required Makefile change to suppress this waning. I would like to make sure that you agree to drop the -Wunused-value warning, before i do so. So please report back. Diederik de Groot wrote: By the way did you check out this 'Posted 5 days, 21 hours ago (March 29, 2015, 1:49 a.m.)' entry on this page. I tried to explain how this warning could be benifial in finding these used after ..._unref issues. rmudgett wrote: Those findings were not real bugs. See my review of those specific findings. Agreed ! So if anybody else thinks we should drop this warning let me know, and we'll drop it. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review15049 --- On April 4, 2015, 1:15 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated April 4, 2015, 1:15 a.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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Just to make it clear, this was just a sample / proposal to work through large set of -Wunused-value warnings return. Some of them could be interesting, others might not be be. Mostly looking for a discussion about which path to follow, and see if we can divvy up the work a little. Compiles / Untested / Pretty certain it contains issue because of different way of dealing with return value from ..._unref. File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 1, 2015, 4:52 a.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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 1, 2015, 4:54 a.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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
On April 1, 2015, 4:30 a.m., rmudgett wrote:
/branches/13/channels/chan_skinny.c, line 2387
https://reviewboard.asterisk.org/r/4533/diff/8/?file=73424#file73424line2387
Guidelines: No space with parens
if (a) {
}
Getting late...
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/#review15013
---
On April 1, 2015, 4:52 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/
---
(Updated April 1, 2015, 4:52 a.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:-Wtautological-compare
Changes:
/branches/13/channels/pjsip/dialplan_functions.c:869
len is of type size_t, which is unsigned. It will not be able to hold a value
0
/branches/13/funcs/func_curl.c:174
Not a 100% sure how to do this correctly. But assiging a negative value is
problematic. Extending the enum in curl/curl.h is not possible either. I
opted to use the enum last entry (CURL_LAST) which is currently not used for
any thing. Another option would be to use one of the OBSOLETE VALUES like 16.
Neither way is very nice though.
/branches/13/include/asterisk/app.h:988
Needed to convey the error state returned by
res/res_stasis_recording.c:stasis_app_recording_if_exists_parse
/branches/13/include/asterisk/cel.h:77
Added to convey not-found or error state. Not sure which name would be
prefered for such an enum value.
/branches/13/main/cel.c:536
Return actual enum instead of -1,l which can not be conveyed by this enum.
/branches/13/main/enum.c:260
dn_expand return signed int
/branches/13/main/event.c:202
enum type cannot be 0
/branches/13/main/indications.c:362
tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not
sure what should happend when freq1 / freq2 are 0 already... (needs recheck
by source owner)
/branches/13/main/presencestate.c:293
Should use the actual enum value for INVALID State
/branches/13/main/security_events.c:432/890/1176/
enum event_type cannot be 0
/branches/13/main/udptl.c:365/649/661
encode_length returns and unsigned int, so checking if 0 does not make
sence. Not 100% if encode_length has side effects, so left the actual call
the this function in place. (Needs to be rechecked by code-owner)
/branches/13/res/res_pjsip_exten_state.c:411
Used a temporary int variable to be able to check the return value from
ast_hint_presence_state.. Not very nice, but did not want to change the
signature of this function.
/branches/13/res/res_stasis_playback.c:634
operation is enum and cannot be 0
/branches/13/res/res_stasis_recording.c:598/604
recording-state / operation is enum and cannot be 0
/branches/13/res/res_security_log.c:992
enum event_type cannot be 0
use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the
lower limit which is always true.
Diffs
-
/branches/13/res/res_stasis_recording.c 433444
/branches/13/res/res_stasis_playback.c 433444
/branches/13/res/res_pjsip_exten_state.c 433444
/branches/13/res/ari/resource_channels.c 433444
/branches/13/res/ari/resource_bridges.c 433444
/branches/13/main/udptl.c 433444
/branches/13/main/security_events.c 433444
/branches/13/main/presencestate.c 433444
/branches/13/main/indications.c 433444
/branches/13/main/event.c 433444
/branches/13/main/enum.c 433444
/branches/13/main/cel.c 433444
/branches/13/main/app.c 433444
/branches/13/include/asterisk/utils.h 433444
/branches/13/include/asterisk/logger.h 433444
/branches/13/include/asterisk/cel.h 433444
/branches/13/include/asterisk/app.h 433444
/branches/13/funcs/func_curl.c 433444
/branches/13/channels/pjsip/dialplan_functions.c 433444
/branches/13/channels/chan_skinny.c 433444
Diff: https://reviewboard.asterisk.org/r/4533/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 4:47 a.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. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
On April 1, 2015, 1:21 a.m., rmudgett wrote:
/branches/13/main/utils.c, lines 492-493
https://reviewboard.asterisk.org/r/4555/diff/5/?file=73346#file73346line492
Revert this. This change could affect the callers of the function
since you are changing the API. However, no callers currently care about
the return value.
You changed this because clang had a problem in test_semi1() in
tests/test_strings.c. There is a better way.
Diederik de Groot wrote:
Actually as i tried to mention in the description i changed this
according to the tests being run. I was not sure which one is supposed to be
leading, the test or the source.
The test says it expects:
test_semi(;, , 0)
To be legal. FOr it to be legal ast_alloca(0) must be allowed.
Am i really changing the API here ?
rmudgett wrote:
You are changing what is returned if buflen is zero. Before it would
return outbuf while the change makes it return NULL. It is just fortunate
that nothing cares about the return value.
The test code is in error here because for a zero length it writes beyond
the buffer boundary.
Ok Agreed.
On April 1, 2015, 1:21 a.m., rmudgett wrote:
/branches/13/tests/test_strings.c, lines 395-396
https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line395
Is clang returning a NULL pointer when passed a zero length? If so
this could be a problem througout the codebase since the code assumes that
the function can never fail.
Diederik de Groot wrote:
I am not sure what happens, the compiler actually segfaults when it
encountered this one. I was a little shocked about it as well. I guess the
__builtins are a little more scary. If you wanna find out, give it a try. I
think this should at least be reported to the llvm/clang team.
I am not sure how to interpret alloca(0) either... What is the developer
trying to achieve here. And what should it return. It can't allocate space of
0 length and return a pointer to it.
A simple but ugly temp-fix would be to change
#define ast_alloca(size) __builtin_alloca(size)
to:
#if defined(__clang)
#define ast_alloca(size) __builtin_alloca(size ? size : 1);
#else
#define ast_alloca(size) __builtin_alloca(size)
#endif
rmudgett wrote:
That could be an undefined area. What is the return value of malloc(0)?
Some systems return NULL while other systems return a pointer that you cannot
dereference but can later pass to free().
The problem with the original test_semi() code is that with a zero length
we dereference any returned pointer to put a '\0' in it. For a zero length
buffer that is beyond the bounds of the buffer and will corrupt the stack.
The code also reads beyond the boundary in strcmp().
Wrote a little tester.c
#include malloc.h
#include stdio.h
#include strings.h
int main() {
int *test1 = malloc(0);
int *test2 = __builtin_alloca(0);
printf(%p / %p\n, test1, test2);
free(test1);
}
And without any compiler flags, clang and gcc seem to be happy and running this
without a problem. Will have to figure out which compiler flags the Makefile
compounds to see if that makes any difference.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4555/#review15000
---
On April 1, 2015, 4:47 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4555/
---
(Updated April 1, 2015, 4:47 a.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.
fixes for tests to be compiled using clang
Diffs
-
/branches/13/tests/test_strings.c 433444
/branches/13/tests/test_stringfields.c 433444
/branches/13/tests/test_sched.c 433444
/branches/13/tests/test_acl.c 433444
Diff: https://reviewboard.asterisk.org/r/4555/diff/
Testing
---
executing the tests one-by-one works fine (completes to end) (skipping
/main/stdtime) -
test show results failed:
=== /main/message/ ==
FAIL test_message_queue_handler_nom /main/message/ 31036ms
[test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test
timed out while waiting for handler to get message
Not sure if this is actually a fail or just
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On April 1, 2015, 12:30 a.m., rmudgett wrote: I dislike this warning. It is more nuisance than helpful. I once worked with compiler that had a similar warning. The lengths you sometimes have to go to eradicate the the warning can be extream. There are several places in this patch that change code behavior and introduce bugs. Hi rmudgett, I agree this would be the biggest amount of work of the whole 'clang' project. It does not really matter from a performance standpoint i think. It can be helpfull to find refcount issues where a previously used refcounted pointer was unreffed and might vanish at any moment. If we where to always require that you have to use the return value from any function including XXX_unref, then we would be setting that that pointer to NULL and therefor the static compiler would be able to detect the null pointer dereference for any use after this location. There are other ways to deal with this, for example adding __attribute(context)__ and using sparse to find these (like the linux-kernel does) instead of depending on this little warning. Is it worth the work ? I don't know. I leave that decision up to you guys. You might want to have a look at: http://asterisk.chan-sccp.net:443/scanbuild-output/2015-03-29-155615-16015-1/ To see how usefull this static compiler can really be (Don't mind the Cast from non-struct type to struct type for now). FYI : I know my patch/fix was certainly not complete nor quite sure not bug free, it was more a sample case of how this could be handled, sorry that that was not clear from the patch description. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14999 --- On March 29, 2015, 1:44 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 1:44 a.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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/tests/test_strings.c, line 399 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line399 Did you mean ast_alloca() or ast_strdupa()? Ooh sorry i meant ast_alloca... Typo. On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/main/utils.c, lines 492-493 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73346#file73346line492 Revert this. This change could affect the callers of the function since you are changing the API. However, no callers currently care about the return value. You changed this because clang had a problem in test_semi1() in tests/test_strings.c. There is a better way. Actually as i tried to mention in the description i changed this according to the tests being run. I was not sure which one is supposed to be leading, the test or the source. The test says it expects: test_semi(;, , 0) To be legal. FOr it to be legal ast_alloca(0) must be allowed. Am i really changing the API here ? On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/tests/test_strings.c, lines 395-396 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line395 Is clang returning a NULL pointer when passed a zero length? If so this could be a problem througout the codebase since the code assumes that the function can never fail. I am not sure what happens, the compiler actually segfaults when it encountered this one. I was a little shocked about it as well. I guess the __builtins are a little more scary. If you wanna find out, give it a try. I think this should at least be reported to the llvm/clang team. I am not sure how to interpret alloca(0) either... What is the developer trying to achieve here. And what should it return. It can't allocate space of 0 length and return a pointer to it. A simple but ugly temp-fix would be to change #define ast_alloca(size) __builtin_alloca(size) to: #if defined(__clang) #define ast_alloca(size) __builtin_alloca(size ? size : 1); #else #define ast_alloca(size) __builtin_alloca(size) #endif On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/tests/test_strings.c, line 393 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line393 Revert this. On April 1, 2015, 1:21 a.m., rmudgett wrote: /branches/13/tests/test_strings.c, lines 407-409 https://reviewboard.asterisk.org/r/4555/diff/5/?file=73351#file73351line407 Revert now that the clang ast_alloca() problem is worked around. ast_alloca problem remain, but is not specifically related to this issue. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/#review15000 --- On April 1, 2015, 3:30 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 3:30 a.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. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 3:30 a.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. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
On April 1, 2015, 3:16 a.m., rmudgett wrote:
/branches/13/funcs/func_curl.c, lines 174-175
https://reviewboard.asterisk.org/r/4533/diff/7/?file=73369#file73369line174
Try defining this as:
#define CURLOPT_SPECIAL_HASHCOMPAT ((CURLoption) -500)
This should shut-up the compiler without changing the binary value.
CurlOption is a positive enum though. So the warning about it not being allowed
to be negative remains, won't it ?
BTW: I made a typo while changing this one
I was meaning to set it to:
#define CURLOPT_SPECIAL_HASHCOMPAT CURLOPT_LASTENTRY
Which is a legal value. Not really happy with this though.
Setting it to
#define CURLOPT_SPECIAL_HASHCOMPAT ((CURLoption) 500)
Might be an option, not sure if this is checked though.
This would require a little further source investigation.
On April 1, 2015, 3:16 a.m., rmudgett wrote:
/branches/13/include/asterisk/utils.h, lines 833-837
https://reviewboard.asterisk.org/r/4533/diff/7/?file=73373#file73373line833
You should be able to do this cast change unconditionally.
Just wanted to denote that it was clang specific.
On April 1, 2015, 3:16 a.m., rmudgett wrote:
/branches/13/res/res_pjsip_exten_state.c, lines 412-413
https://reviewboard.asterisk.org/r/4533/diff/7/?file=73384#file73384line412
Pull the assignment within the test out.
presence_state = ast_hint_presence_state();
if (presence_state == -1) {
}
I have a feeling that presence_state should be checked for
AST_PRESENCE_INVALID as an error in addition to -1. However, I'm not sure.
Added a check for both, seems right. Would have to be checked though, by
someone who knows !
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/#review15003
---
On March 30, 2015, 1:08 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4533/
---
(Updated March 30, 2015, 1:08 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:-Wtautological-compare
Changes:
/branches/13/channels/pjsip/dialplan_functions.c:869
len is of type size_t, which is unsigned. It will not be able to hold a value
0
/branches/13/funcs/func_curl.c:174
Not a 100% sure how to do this correctly. But assiging a negative value is
problematic. Extending the enum in curl/curl.h is not possible either. I
opted to use the enum last entry (CURL_LAST) which is currently not used for
any thing. Another option would be to use one of the OBSOLETE VALUES like 16.
Neither way is very nice though.
/branches/13/include/asterisk/app.h:988
Needed to convey the error state returned by
res/res_stasis_recording.c:stasis_app_recording_if_exists_parse
/branches/13/include/asterisk/cel.h:77
Added to convey not-found or error state. Not sure which name would be
prefered for such an enum value.
/branches/13/main/cel.c:536
Return actual enum instead of -1,l which can not be conveyed by this enum.
/branches/13/main/enum.c:260
dn_expand return signed int
/branches/13/main/event.c:202
enum type cannot be 0
/branches/13/main/indications.c:362
tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not
sure what should happend when freq1 / freq2 are 0 already... (needs recheck
by source owner)
/branches/13/main/presencestate.c:293
Should use the actual enum value for INVALID State
/branches/13/main/security_events.c:432/890/1176/
enum event_type cannot be 0
/branches/13/main/udptl.c:365/649/661
encode_length returns and unsigned int, so checking if 0 does not make
sence. Not 100% if encode_length has side effects, so left the actual call
the this function in place. (Needs to be rechecked by code-owner)
/branches/13/res/res_pjsip_exten_state.c:411
Used a temporary int variable to be able to check the return value from
ast_hint_presence_state.. Not very nice, but did not want to change the
signature of this function.
/branches/13/res/res_stasis_playback.c:634
operation is enum and cannot be 0
/branches/13/res/res_stasis_recording.c:598/604
recording-state / operation is enum and cannot be 0
/branches/13/res/res_security_log.c:992
enum event_type cannot be 0
use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the
lower limit which is always true.
Diffs
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 4:46 a.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. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4550/ --- (Updated April 1, 2015, 3:33 a.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:--dev-mode and -Wparentheses-equality include/asterisk/linkedlists.h:450 == Got rid of the extraneous () in the comparison to NULL by negating the comparison include/asterisk/vector.h:261 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem) != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:467 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem)-format != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:492 = Because of the () where removed previously, they are now needed here. main/stasis_message_router.c:82 === Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem).message_type != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/stdtime/localtime.c:197/208 Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((sp)-wd[0] != SP_STACK_FLAG) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. more of the same for include/asterisk/dlinkedlists.h:422/431/469 main/astobj2_hash.c:768 - Maybe one of you has a better/nicer solution. Diffs (updated) - /branches/13/tests/test_linkedlists.c 433444 /branches/13/main/stdtime/localtime.c 433444 /branches/13/main/stasis_message_router.c 433444 /branches/13/main/format_cap.c 433444 /branches/13/main/astobj2_hash.c 433444 /branches/13/include/asterisk/vector.h 433444 /branches/13/include/asterisk/linkedlists.h 433444 /branches/13/include/asterisk/dlinkedlists.h 433444 Diff: https://reviewboard.asterisk.org/r/4550/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated April 1, 2015, 4:42 a.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. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
On April 1, 2015, 3:16 a.m., rmudgett wrote: Thanks for your help on this one ! - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/#review15003 --- On April 1, 2015, 3:59 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 1, 2015, 3:59 a.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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated April 1, 2015, 3:59 a.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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4541: clang compiler warning: -Wpointer-bool-conversion
On March 31, 2015, 5:50 p.m., Mark Michelson wrote:
/branches/13/apps/app_queue.c, lines 9089-9093
https://reviewboard.asterisk.org/r/4541/diff/1/?file=73002#file73002line9089
This change is not correct. While the mem-state_interface will always
be non-NULL (thanks, clang!), it may not have been configured and will
therefore be zero-length.
The correct fix here is to switch the previous
if (mem-state_interface)
to be
if (ast_strlen_zero(mem-state_interface))
Thanks for pointing that one out.
Fixed using if (!ast_strlen_zero(mem-state_interface)) instead :-)
On March 31, 2015, 5:50 p.m., Mark Michelson wrote:
/branches/13/res/res_smdi.c, lines 876-881
https://reviewboard.asterisk.org/r/4541/diff/1/?file=73004#file73004line876
This change is incorrect. The if checks should be changed to
if (ast_strlen_zero(search_msg-mesg_dsk_num))
and
if (ast_strlen_zero(search_msg-mesg_desk_term))
Fixed using if (!ast_strlen_zero(...)) instead.
Thanks Mark !
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/#review14978
---
On March 31, 2015, 6:05 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated March 31, 2015, 6:05 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:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term'
will always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4541: clang compiler warning: -Wpointer-bool-conversion
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated March 31, 2015, 6:05 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:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs (updated)
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
On March 31, 2015, 7:14 p.m., Mark Michelson wrote:
I don't understand the purpose of this warning. I tried searching for
details about this warning flag on the internet and came up empty, so I
can't find any documentation that explains what type of error this check is
supposed to help avoid.
It appears that the warning is intended to get rid of extra parentheses
where they are unnecessary. The problem is that I don't see anything wrong
with having them there, especially in macro definitions.
Diederik de Groot wrote:
Ok you know that to prevent an unintendet assignment where a comparisson
was indended compilers request you to write
/* comparisson */
if (x == 1) {
}
but
/* assignment = most modern compilers will complain*/
if (x = 1) { /* = warning raised*/
}
so you are forced to write
if ((x = 1)) {
}
instead, to guarantee that you meant to assign a value here.
But most compilers will not complain if you write the first comparison
between double parantheses
if ((x == 1)) {
}
Which is perfectly legal, as you pointed out.
But a maintainer of the code might later be in doubt as to what you mean.
Was an assignment was not intended by the original writer (non-obvious).
clang can be made complain about this (-Wparantheses-equalty or -Wall
-Werror) to make sure this last one is not allowed.
So this can be considered the reverse of the first warning, where an
assignment requires double parantheses.
rmudgett wrote:
I think this warning is a backlash from the warning about putting
assignments inside tests that you suppress by adding parentheses.
if ((a = b))
At any rate this is a very bad warning and should not be used.
The parentheses in macros are to prevent unexpected precedence operator
bindings:
#define BAD_MACRO(a,b) a + b
if (BAD_MACRO(a, b) * 3 != (a + b) * 3) printf(ERROR unexpected
result\n);
@rmudgett: I agreed, that's why i was asking in my initial description, if
anybody knew of a better way to solve both the macro issue but still satisfy
the parantheses warning. Without having to suppress it. Something ugly to fix
the macro expansion is to use a double negating comparison or maybe even an
inline comparison return TRUE/FALSE. Both of which did not really strike me as
particularly nice.
I can live with suppressing this warning, but it would be nice if somebody knew
of a nice way to have it both ways.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
---
On March 29, 2015, 7:14 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/
---
(Updated March 29, 2015, 7:14 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:--dev-mode and -Wparentheses-equality
include/asterisk/linkedlists.h:450
==
Got rid of the extraneous () in the comparison to NULL by negating the
comparison
include/asterisk/vector.h:261
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison !((elem)
!= (value)) which would keep everything encapsulated, but does result in a
double negative, which is ugly as well.
main/format_cap.c:467
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((elem)-format != (value)) which would keep everything encapsulated, but
does result in a double negative, which is ugly as well.
main/format_cap.c:492
=
Because of the () where removed previously, they are now needed here.
main/stasis_message_router.c:82
===
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double
Re: [asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
On March 31, 2015, 7:14 p.m., Mark Michelson wrote:
I don't understand the purpose of this warning. I tried searching for
details about this warning flag on the internet and came up empty, so I
can't find any documentation that explains what type of error this check is
supposed to help avoid.
It appears that the warning is intended to get rid of extra parentheses
where they are unnecessary. The problem is that I don't see anything wrong
with having them there, especially in macro definitions.
Diederik de Groot wrote:
Ok you know that to prevent an unintendet assignment where a comparisson
was indended compilers request you to write
/* comparisson */
if (x == 1) {
}
but
/* assignment = most modern compilers will complain*/
if (x = 1) { /* = warning raised*/
}
so you are forced to write
if ((x = 1)) {
}
instead, to guarantee that you meant to assign a value here.
But most compilers will not complain if you write the first comparison
between double parantheses
if ((x == 1)) {
}
Which is perfectly legal, as you pointed out.
But a maintainer of the code might later be in doubt as to what you mean.
Was an assignment was not intended by the original writer (non-obvious).
clang can be made complain about this (-Wparantheses-equalty or -Wall
-Werror) to make sure this last one is not allowed.
So this can be considered the reverse of the first warning, where an
assignment requires double parantheses.
rmudgett wrote:
I think this warning is a backlash from the warning about putting
assignments inside tests that you suppress by adding parentheses.
if ((a = b))
At any rate this is a very bad warning and should not be used.
The parentheses in macros are to prevent unexpected precedence operator
bindings:
#define BAD_MACRO(a,b) a + b
if (BAD_MACRO(a, b) * 3 != (a + b) * 3) printf(ERROR unexpected
result\n);
Diederik de Groot wrote:
@rmudgett: I agreed, that's why i was asking in my initial description,
if anybody knew of a better way to solve both the macro issue but still
satisfy the parantheses warning. Without having to suppress it. Something
ugly to fix the macro expansion is to use a double negating comparison or
maybe even an inline comparison return TRUE/FALSE. Both of which did not
really strike me as particularly nice.
I can live with suppressing this warning, but it would be nice if
somebody knew of a nice way to have it both ways.
rmudgett wrote:
Please discard this review with predjudice. I see no benefit to this
-Wparentheses-equality option.
WOuld have been nice if this Warning would not react to results coming from
macro's (so only plain code), but alas. I guess the AST does not have that last
little detail.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
---
On March 29, 2015, 7:14 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/
---
(Updated March 29, 2015, 7:14 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:--dev-mode and -Wparentheses-equality
include/asterisk/linkedlists.h:450
==
Got rid of the extraneous () in the comparison to NULL by negating the
comparison
include/asterisk/vector.h:261
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison !((elem)
!= (value)) which would keep everything encapsulated, but does result in a
double negative, which is ugly as well.
main/format_cap.c:467
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((elem)-format != (value)) which would keep everything encapsulated, but
does result in a double negative, which is ugly as well.
main/format_cap.c:492
Re: [asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
On March 31, 2015, 7:14 p.m., Mark Michelson wrote:
I don't understand the purpose of this warning. I tried searching for
details about this warning flag on the internet and came up empty, so I
can't find any documentation that explains what type of error this check is
supposed to help avoid.
It appears that the warning is intended to get rid of extra parentheses
where they are unnecessary. The problem is that I don't see anything wrong
with having them there, especially in macro definitions.
Ok you know that to prevent an unintendet assignment where a comparisson was
indended compilers request you to write
/* comparisson */
if (x == 1) {
}
but
/* assignment = most modern compilers will complain*/
if (x = 1) { /* = warning raised*/
}
so you are forced to write
if ((x = 1)) {
}
instead, to guarantee that you meant to assign a value here.
But most compilers will not complain if you write the first comparison between
double parantheses
if ((x == 1)) {
}
Which is perfectly legal, as you pointed out.
But a maintainer of the code might later be in doubt as to what you mean. Was
an assignment was not intended by the original writer (non-obvious).
clang can be made complain about this (-Wparantheses-equalty or -Wall -Werror)
to make sure this last one is not allowed.
So this can be considered the reverse of the first warning, where an assignment
requires double parantheses.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
---
On March 29, 2015, 7:14 p.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/
---
(Updated March 29, 2015, 7:14 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:--dev-mode and -Wparentheses-equality
include/asterisk/linkedlists.h:450
==
Got rid of the extraneous () in the comparison to NULL by negating the
comparison
include/asterisk/vector.h:261
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison !((elem)
!= (value)) which would keep everything encapsulated, but does result in a
double negative, which is ugly as well.
main/format_cap.c:467
=
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((elem)-format != (value)) which would keep everything encapsulated, but
does result in a double negative, which is ugly as well.
main/format_cap.c:492
=
Because of the () where removed previously, they are now needed here.
main/stasis_message_router.c:82
===
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((elem).message_type != (value)) which would keep everything encapsulated,
but does result in a double negative, which is ugly as well.
main/stdtime/localtime.c:197/208
Removed the extraneous (). Not particularly happy about this though as they
where used to keep this macro encapsulated (Does however not cause any
compile issues)
Another possible solutions would be to double negate the comparison
!((sp)-wd[0] != SP_STACK_FLAG) which would keep everything encapsulated, but
does result in a double negative, which is ugly as well.
more of the same for
include/asterisk/dlinkedlists.h:422/431/469
main/astobj2_hash.c:768
-
Maybe one of you has a better/nicer solution.
Diffs
-
/branches/13/main/stdtime/localtime.c 433444
/branches/13/main/stasis_message_router.c 433444
/branches/13/main/format_cap.c 433444
/branches/13/main/astobj2_hash.c 433444
/branches/13/include/asterisk/vector.h 433444
/branches/13/include/asterisk/linkedlists.h 433444
/branches/13/include/asterisk/dlinkedlists.h 433444
Re: [asterisk-dev] [Code Review] 4541: clang compiler warning: -Wpointer-bool-conversion
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4541/
---
(Updated March 31, 2015, 7:15 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:-Wpointer-bool-conversion
Issues:
chan_pjsip.c:1898:13: warning: address of array 'data-text' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (!data-text) {
~~~^~~~
app_minivm.c:1841:18: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_minivm.c:1871:17: warning: address of array 'etemplate-locale' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (etemplate-locale) {
~~ ~~~^~
app_queue.c:6686:19: warning: address of array 'qe-parent-monfmt' will always
evaluate to 'true' [-Wpointer-bool-conversion]
if (qe-parent-monfmt *qe-parent-monfmt) {
^~ ~~
app_queue.c:9090:15: warning: address of array 'mem-state_interface' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (mem-state_interface) {
~~ ~^~~
res_smdi.c:876:19: warning: address of array 'search_msg-mesg_desk_num' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_num) {
~~ ^
res_smdi.c:879:19: warning: address of array 'search_msg-mesg_desk_term' will
always evaluate to 'true' [-Wpointer-bool-conversion]
if (search_msg-mesg_desk_term) {
~~ ^~
Changed: removed the superfluous checks
Diffs (updated)
-
/branches/13/res/res_smdi.c 433444
/branches/13/channels/chan_pjsip.c 433444
/branches/13/apps/app_queue.c 433444
/branches/13/apps/app_minivm.c 433444
Diff: https://reviewboard.asterisk.org/r/4541/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated March 30, 2015, 1:08 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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4551: clang compiler warning: -Wnon-literal-null-conversion
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4551/ --- (Updated March 30, 2015, 1:16 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:-Wnon-literal-null-conversion Diffs (updated) - /branches/13/res/stasis_recording/stored.c 433444 /branches/13/channels/chan_skinny.c 433444 /branches/13/channels/chan_sip.c 433444 Diff: https://reviewboard.asterisk.org/r/4551/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4535: clang compiler warning: -Wenum-conversion
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4535/ --- (Updated March 29, 2015, 9:39 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433747 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:-Wenum-conversion Changes: /branches/13/channels/chan_pjsip.c:923 Wrong enum being used /branches/13/channels/chan_sip.c:19102 Incorrect enum /branches/13/channels/chan_sip.c:19107 Incorrect enum returned /branches/13/include/asterisk/strings.h:1236 Replaced enum ao2_container_opts opts - enum ao2_alloc_opts opts Not 100% sure if this substition is correct. Please verify. /branches/13/main/strings.c:173 Replaced enum ao2_container_opts opts - enum ao2_alloc_opts opts Not 100% sure if this substition is correct. Please verify. /branches/13/res/res_stasis.c:1803 Incorrect enum for return value Diffs - /branches/13/res/res_stasis.c 433444 /branches/13/main/strings.c 433444 /branches/13/include/asterisk/strings.h 433444 /branches/13/channels/chan_sip.c 433444 /branches/13/channels/chan_pjsip.c 433444 Diff: https://reviewboard.asterisk.org/r/4535/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4525: clang compiler warning: -Wabsolute-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4525/ --- (Updated March 29, 2015, 9:44 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433749 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:-Wabsolute-value Changes: /branches/13/apps/app_queue.c:3687 int - long for avgholdmins, avgholdsecs; /branches/13/apps/app_queue.c:6581 int - long for holdtime, holdtimesecs; /branches/13/channels/chan_iax2.c:6080 Moved adjust here, so it can help out in casting (ms - p-nextpred) to int /branches/13/res/res_calendar.c:1109 Not sure if this is prefered over downcasting to int Diffs - /branches/13/res/res_rtp_asterisk.c 433444 /branches/13/res/res_calendar.c 433444 /branches/13/main/jitterbuf.c 433444 /branches/13/channels/sip/dialplan_functions.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_queue.c 433444 Diff: https://reviewboard.asterisk.org/r/4525/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4530: clang compiler warning: -Wgnu-variable-sized-type-not-at-end
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4530/ --- (Updated March 29, 2015, 8:52 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433717 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\nclang compiler warning:-Wgnu-variable-sized-type-not-at-end Diffs - /branches/13/main/stdtime/localtime.c 433444 Diff: https://reviewboard.asterisk.org/r/4530/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4545: clang compiler warning: -Wunused-command-line-argument
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4545/ --- (Updated March 29, 2015, 8:56 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433720 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-command-line-argument Diffs - /branches/13/Makefile.rules 433444 Diff: https://reviewboard.asterisk.org/r/4545/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4530: clang compiler warning: -Wgnu-variable-sized-type-not-at-end
On March 30, 2015, 4:11 a.m., Matt Jordan wrote:
So, small problem with this change. When compiling with gcc 4.8.2, I get
the following warning:
In function ‘read’,
inlined from ‘inotify_daemon’ at stdtime/localtime.c:380:6:
/usr/include/x86_64-linux-gnu/bits/unistd.h:42:2: error: call to
‘__read_chk_warn’ declared with attribute warning: read called with bigger
length than size of the destination buffer [-Werror]
return __read_chk_warn (__fd, __buf, __nbytes, __bos0 (__buf));
Interestingly, the build agents didn't kick this back. But we'll need to
find a way to get this fixed for gcc as well.
Matt Jordan wrote:
So, this is interesting. Looking at unistd.h:
extern ssize_t __read_chk (int __fd, void *__buf, size_t __nbytes,
size_t __buflen) __wur;
extern ssize_t __REDIRECT (__read_alias, (int __fd, void *__buf,
size_t __nbytes), read) __wur;
extern ssize_t __REDIRECT (__read_chk_warn,
(int __fd, void *__buf, size_t __nbytes,
size_t __buflen), __read_chk)
__wur __warnattr (read called with bigger length than size of
the destination buffer);
__fortify_function __wur ssize_t
read (int __fd, void *__buf, size_t __nbytes)
{
if (__bos0 (__buf) != (size_t) -1)
{
if (!__builtin_constant_p (__nbytes))
return __read_chk (__fd, __buf, __nbytes, __bos0 (__buf));
if (__nbytes __bos0 (__buf))
return __read_chk_warn (__fd, __buf, __nbytes, __bos0 (__buf));
}
return __read_alias (__fd, __buf, __nbytes);
}
That is, if __nbytes is greater than the result of GCC's built-in object
size (https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html) for the
struct, we'll kick back a warning.
As it turns out, this is because there is an error in the code here -
we're passing the address of the pointer to the struct, not iev, which is a
pointer to the struct. Hence, the number of bytes is probably going to be lot
larger than the number of bytes that make up a pointer! Changing this to just
read from the pointer to the struct fixes the warning.
I guess i was running into the same problem whem running the tests/test_time.c
using the clang compiled version. It segfaulted in inotify_daemon as wel.
Thanks for finding/fixing this one. Really making progress here.
- Diederik
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4530/#review14961
---
On March 30, 2015, 3:52 a.m., Diederik de Groot wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4530/
---
(Updated March 30, 2015, 3:52 a.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\nclang compiler warning:-Wgnu-variable-sized-type-not-at-end
Diffs
-
/branches/13/main/stdtime/localtime.c 433444
Diff: https://reviewboard.asterisk.org/r/4530/diff/
Testing
---
Thanks,
Diederik de Groot
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4543: clang compiler warning: RAII-clang reentrancy / Updating variable from inside a _block
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/ --- (Updated March 29, 2015, 2:57 p.m.) Review request for Asterisk Developers. Changes --- Ran clang's static analyzer which found 819 of potential issues, some of which are real bugs and of course some are also false positives. Adding both clang and scan-build to a CI environment could be quite helpfull in tracking down these bugs. The scanbuild file is pretty bif (sorry about that), but it includes the branching path that leads up to the bug. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- RAII-clang reentrancy / Updating variable from inside a _block To update the varname variable from inside the cleanup block it needs to be decorated with __block as in: __block vartype varname = ctor; Diffs - /branches/13/include/asterisk/utils.h 433444 /branches/13/contrib/scan-build PRE-CREATION /branches/13/configure.ac 433444 /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4543/diff/ Testing --- File Attachments (updated) scanbuild output https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/5417a9e0-0023-4eb2-aa97-5a2659997e68__scanbuild_2015-03-29_r433715.tar.bz2 Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4550/ --- (Updated March 29, 2015, 7:14 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description (updated) --- 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:--dev-mode and -Wparentheses-equality include/asterisk/linkedlists.h:450 == Got rid of the extraneous () in the comparison to NULL by negating the comparison include/asterisk/vector.h:261 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem) != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:467 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem)-format != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:492 = Because of the () where removed previously, they are now needed here. main/stasis_message_router.c:82 === Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem).message_type != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/stdtime/localtime.c:197/208 Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((sp)-wd[0] != SP_STACK_FLAG) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. more of the same for include/asterisk/dlinkedlists.h:422/431/469 main/astobj2_hash.c:768 - Maybe one of you has a better/nicer solution. Diffs - /branches/13/main/stdtime/localtime.c 433444 /branches/13/main/stasis_message_router.c 433444 /branches/13/main/format_cap.c 433444 /branches/13/main/astobj2_hash.c 433444 /branches/13/include/asterisk/vector.h 433444 /branches/13/include/asterisk/linkedlists.h 433444 /branches/13/include/asterisk/dlinkedlists.h 433444 Diff: https://reviewboard.asterisk.org/r/4550/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- 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:fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/#review14959 --- /branches/13/tests/test_acl.c https://reviewboard.asterisk.org/r/4555/#comment25579 clang can't handle gcc's nested function extension. I could have opted to build a clang ^block instead, but i choose the simple external function instead. /branches/13/tests/test_strings.c https://reviewboard.asterisk.org/r/4555/#comment25578 The clang compiler blows up/crashes when running ast_alloca(0) (ie:__builtin_alloca(0)) possible solutions: - report issue to clang / llvm - add check to ast_alloca and enforce return value on error - or make sure we don't call ast_alloca with a size of 0 ever. - Diederik de Groot On March 29, 2015, 7:26 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 29, 2015, 7:26 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. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4550/ --- (Updated March 29, 2015, 7:11 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:--dev-mode and -Wparentheses-equality include/asterisk/linkedlists.h:450 == Got rid of the extraneous () in the comparison to NULL by negating the comparison include/asterisk/vector.h:261 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem) != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:467 = Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem)-format != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/format_cap.c:492 = Because of the () where removed previously, they are now needed here. main/stasis_message_router.c:82 === Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((elem).message_type != (value)) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. main/stdtime/localtime.c:197/208 Removed the extraneous (). Not particularly happy about this though as they where used to keep this macro encapsulated (Does however not cause any compile issues) Another possible solutions would be to double negate the comparison !((sp)-wd[0] != SP_STACK_FLAG) which would keep everything encapsulated, but does result in a double negative, which is ugly as well. - Maybe one of you has a better/nicer solution. Diffs (updated) - /branches/13/main/stdtime/localtime.c 433444 /branches/13/main/stasis_message_router.c 433444 /branches/13/main/format_cap.c 433444 /branches/13/main/astobj2_hash.c 433444 /branches/13/include/asterisk/vector.h 433444 /branches/13/include/asterisk/linkedlists.h 433444 /branches/13/include/asterisk/dlinkedlists.h 433444 Diff: https://reviewboard.asterisk.org/r/4550/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 29, 2015, 7:24 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. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/#review14957 --- /branches/13/tests/test_acl.c https://reviewboard.asterisk.org/r/4555/#comment25574 Can't use the gcc nested function extension. Could have replaced it with a clang -block solution, but opted for the simple external function solution. /branches/13/tests/test_strings.c https://reviewboard.asterisk.org/r/4555/#comment25575 The clang compiler blows up/crashes when running ast_alloca(0) (ie:__builtin_alloca(0)) possible solutions: - report issue to clang / llvm - add check to ast_alloca and enforce return value on error - or make sure we don't call ast_alloca with a size of 0 ever. /branches/13/tests/test_strings.c https://reviewboard.asterisk.org/r/4555/#comment25576 Woops. Debugging remnant, should not have been here. - Diederik de Groot On March 29, 2015, 7:24 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 29, 2015, 7:24 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. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated March 29, 2015, 7:10 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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:432/890/1176/ enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 /branches/13/res/res_security_log.c:992 enum event_type cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/logger.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 /branches/13/channels/chan_skinny.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 29, 2015, 7:26 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. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 29, 2015, 7:20 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description (updated) --- 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. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 30, 2015, 12:42 a.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. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 /branches/13/main/utils.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 30, 2015, 12:43 a.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. fixes for tests to be compiled using clang Diffs (updated) - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 /branches/13/main/utils.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/#review14960 --- /branches/13/tests/test_strings.c https://reviewboard.asterisk.org/r/4555/#comment25581 If buflen being 0 is allowed in these tests, then ast_escape_semicolons returning NULL should also be allowed to make this test a valid case /branches/13/main/utils.c https://reviewboard.asterisk.org/r/4555/#comment25580 If buflen 0 is supposed to be allowed (as suggested by the tests), then this should be checked and not raise a problem in the assert - Diederik de Groot On March 30, 2015, 12:43 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 30, 2015, 12:43 a.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. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 /branches/13/main/utils.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4555: clang compiler warning: fixes for tests to be compiled using clang
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4555/ --- (Updated March 30, 2015, 12:49 a.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. fixes for tests to be compiled using clang Diffs - /branches/13/tests/test_strings.c 433444 /branches/13/tests/test_stringfields.c 433444 /branches/13/tests/test_sched.c 433444 /branches/13/tests/test_linkedlists.c 433444 /branches/13/tests/test_acl.c 433444 /branches/13/main/utils.c 433444 Diff: https://reviewboard.asterisk.org/r/4555/diff/ Testing (updated) --- executing the tests one-by-one works fine (completes to end) (skipping /main/stdtime) - test show results failed: === /main/message/ == FAIL test_message_queue_handler_nom /main/message/ 31036ms [test_message.c:int handler_wait_for_message(struct ast_test *):244]: Test timed out while waiting for handler to get message Not sure if this is actually a fail or just a timeout. WIP === /main/strings/ == FAIL escape_semicolons /main/strings/ 1ms [Mar 29 20:13:43] ERROR[2521]: utils.c:493 char *ast_escape_semicolons(const char *, char *, int): FRACK!, Failed assertion string != NULL outbuf != NULL (0) - explainable by the change made to the source. ast_alloca(0) is not being executed - test2 = NULL: need to resolv the open question how to handle ast_alloca(0) before making any further changes. (With revision 5 of this code, this test now passes without a problem, had to fix both the test and the function being tested though) === /main/stdtime == test execute all fails, caused by the /main/stdtime/ test. START /main/stdtime/ - timezone_watch [test_time.c:enum ast_test_result_state test_timezone_watch(struct ast_test_info *, enum ast_test_command, struct ast_test *):84]: Executing deletion test... j62747*CLI CLI becomes unresponsive / no further command completion for example. Guess this will need a little further investigation. Maybe the source changes made to main/stdtime/ where not completely correct. Seems to be caused by inotify_daemon, at least there is where the segfault happens. WIP File Attachments tests results xml (except /main/stdtime) https://reviewboard.asterisk.org/media/uploaded/files/2015/03/29/4a17471b-4952-43cd-b015-92d00da2338b__tests.xml Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- 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 Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 12:46 a.m.) Review request for Asterisk Developers. Changes --- ./configure --enable-dev-mode CC=clang CXX=clang++ ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 make.report 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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments (updated) ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
On March 29, 2015, 1:49 a.m., Diederik de Groot wrote: The fixes for unused-value to chan_iax2, to compile smoothly should be seen as a showcase that this can be off potential benefit. Would have been nice if ao2_ref(..., -1) would have behaved in the same way, so that ao2_ref(..., -1) would return NULL, maybe this could be a future enhancement. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14952 --- On March 29, 2015, 1:44 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 1:44 a.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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 1:44 a.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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs (updated) - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14952 --- /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25567 If this is the last peer, then releasing it here would mean that peer-pokeexpire would cause a segfault. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25569 using peer = peer_unref(peer), will cause the segfault in the lines below everytime, which should be considered a good thing. Will show use of a released pointer during testing. /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25568 potential segfault because of released peer /branches/13/channels/chan_iax2.c https://reviewboard.asterisk.org/r/4554/#comment25570 function is being called with (peer = 0x0), which showed up after messing with the code above. Either this is a real issue, or caused by fiddling with the code. - Diederik de Groot On March 29, 2015, 1:44 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 29, 2015, 1:44 a.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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- File Attachments ASTCFLAGS=-Wno-error=unused-value make | grep [-W -B1 -A2 https://reviewboard.asterisk.org/media/uploaded/files/2015/03/28/83f2f382-9ef2-41cf-8e7a-c188c014c17e__make.report Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 28, 2015, 8:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description (updated) --- 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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4543: clang compiler warning: RAII-clang reentrancy / Updating variable from inside a _block
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/#review14955 --- /branches/13/contrib/scan-build https://reviewboard.asterisk.org/r/4543/#comment25573 Added a simple script to contrib, to show how to perform a scan-build using clang/llvm-gcc which uses the extensive llvm static analyzer. An html report will be generated in the created scanbuild-output subdirectory - Diederik de Groot On March 29, 2015, 5:01 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/ --- (Updated March 29, 2015, 5:01 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- RAII-clang reentrancy / Updating variable from inside a _block To update the varname variable from inside the cleanup block it needs to be decorated with __block as in: __block vartype varname = ctor; Diffs - /branches/13/include/asterisk/utils.h 433444 /branches/13/contrib/scan-build PRE-CREATION /branches/13/configure.ac 433444 /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4543/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4543: clang compiler warning: RAII-clang reentrancy / Updating variable from inside a _block
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/ --- (Updated March 29, 2015, 5:18 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- RAII-clang reentrancy / Updating variable from inside a _block To update the varname variable from inside the cleanup block it needs to be decorated with __block as in: __block vartype varname = ctor; Diffs (updated) - /branches/13/include/asterisk/utils.h 433444 /branches/13/contrib/scan-build PRE-CREATION /branches/13/configure.ac 433444 /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4543/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4543: clang compiler warning: RAII-clang reentrancy / Updating variable from inside a _block
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/ --- (Updated March 29, 2015, 5:01 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- RAII-clang reentrancy / Updating variable from inside a _block To update the varname variable from inside the cleanup block it needs to be decorated with __block as in: __block vartype varname = ctor; Diffs (updated) - /branches/13/include/asterisk/utils.h 433444 /branches/13/contrib/scan-build PRE-CREATION /branches/13/configure.ac 433444 /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4543/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4543: clang compiler warning: RAII-clang reentrancy / Updating variable from inside a _block
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/#review14954 --- /branches/13/include/asterisk/utils.h https://reviewboard.asterisk.org/r/4543/#comment25572 superfluous null pointer check removed after some testing /branches/13/configure.ac https://reviewboard.asterisk.org/r/4543/#comment25571 move the raii compiler checks to their own m4 file and called the checking function a little earlier in the configure process - Diederik de Groot On March 29, 2015, 4:57 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/ --- (Updated March 29, 2015, 4:57 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- RAII-clang reentrancy / Updating variable from inside a _block To update the varname variable from inside the cleanup block it needs to be decorated with __block as in: __block vartype varname = ctor; Diffs - /branches/13/include/asterisk/utils.h 433444 /branches/13/contrib/scan-build PRE-CREATION /branches/13/configure.ac 433444 /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4543/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4543: clang compiler warning: RAII-clang reentrancy / Updating variable from inside a _block
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4543/ --- (Updated March 29, 2015, 4:57 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- RAII-clang reentrancy / Updating variable from inside a _block To update the varname variable from inside the cleanup block it needs to be decorated with __block as in: __block vartype varname = ctor; Diffs (updated) - /branches/13/include/asterisk/utils.h 433444 /branches/13/contrib/scan-build PRE-CREATION /branches/13/configure.ac 433444 /branches/13/autoconf/ast_check_raii.m4 PRE-CREATION Diff: https://reviewboard.asterisk.org/r/4543/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4554: clang compiler warning: -Wunused-value
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/#review14951 --- Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Will wait for you guys to respond, before continuing. Will needs some more people to finish the '-Wunused-value' task, would be way to much for one person. - Diederik de Groot On March 28, 2015, 8:32 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4554/ --- (Updated March 28, 2015, 8:32 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 Changes: apps/app_agent_pool.c multiple occasions where return value from ast_channel_ref and ast_channel_unref is not checked not used. return value from AST_LIST_REMOVE should be checked. Possible issues with leaked logged reference (added remark) channels/chan_iax2.c return values from ast_callid_ref and ast_callid_unref can/should be used to update the pointer return value from AST_SCHED_DEL should be used to update the scheduled variable return value from AST_LIST_REMOVE should be checked. channels/iax2/parser.c return value from AST_LIST_REMOVE should be checked. include/asterisk/stringfields.h Quick fix to send the returned value from ast_string_field_ptr_set into the void. Work in Progress: There are too many sources issues with -Wunused-value for one person to create all the fixes. If we want to actually use this warning we will need a couple of extra hands-on. Should we suppress -Wunused-value ? : I think some of the issues pointed out by clang could be very helpfull, for example the unref/ref/AST_LIST_REMOVE changes that where needed in chan_iax2.c and apps/app_agent_pool.c. Some others a less interesting, for example the SCHED_DEL fixes. Diffs - /branches/13/include/asterisk/stringfields.h 433444 /branches/13/channels/iax2/parser.c 433444 /branches/13/channels/chan_iax2.c 433444 /branches/13/apps/app_agent_pool.c 433444 Diff: https://reviewboard.asterisk.org/r/4554/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4539: clang compiler warning: -Winitializer-overrides
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4539/ --- (Updated March 28, 2015, 7:27 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433682 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:-Winitializer-overrides Issue: res_pjsip/config_transport.c:382:25: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides] [PJSIP_TLSV1_METHOD] = tlsv1, res_pjsip/config_transport.c:380:31: note: previous initialization is here [PJSIP_SSL_DEFAULT_METHOD] = default, Possible Solutions: 1. PJSIP_SSL_DEFAULT_METHOD is defined as PJSIP_TLSV1_METHOD - remove the offending initializer in line 380 2. define PJSIP_SSL_DEFAULT_METHOD locally (overriding the default define from /usr/include/pjsip/sip_transport_tls.h) and use a different value for it. Not sure which method would be preferred, leaving that decision to the code owner. Diffs - /branches/13/res/res_pjsip/config_transport.c 433444 Diff: https://reviewboard.asterisk.org/r/4539/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4531: clang compiler warning: -Wparentheses-equality
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4531/ --- (Updated March 28, 2015, 7:39 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433687 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:-Wparentheses-equality Changes: Needed to suppress this warning for the generated file 'ael.tab.c' over which we don't have control Diffs - /branches/13/utils/Makefile 433444 /branches/13/res/Makefile 433444 /branches/13/main/channel.c 433444 /branches/13/apps/app_voicemail.c 433444 /branches/13/apps/app_dictate.c 433444 Diff: https://reviewboard.asterisk.org/r/4531/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4526: clang compiler warning: -Wunused-value -Wunused-variable -Wunused-const-variable
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4526/ --- (Updated March 28, 2015, 7:54 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433693 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 -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4546: clang compiler warning: -Warray-bounds
On March 28, 2015, 1:28 p.m., Diederik de Groot wrote: /branches/13/configure.ac, lines 1179-1183 https://reviewboard.asterisk.org/r/4546/diff/2/?file=73163#file73163line1179 Test is not working as i hoped yet. Tests inside configure don't seem to get optimized to the same level (-O3), making this test somewhat useless. Trying to find out how to get the same behaviour as wenn running make. references: https://llvm.org/bugs/show_bug.cgi?id=11536 Will use the 'example.c' source from this bugreport to detect this behaviour. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/#review14933 --- On March 28, 2015, 12:14 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/ --- (Updated March 28, 2015, 12:14 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:-Warray-bounds Diffs - /branches/13/configure.ac 433444 Diff: https://reviewboard.asterisk.org/r/4546/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4546: clang compiler warning: -Warray-bounds
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/#review14935 --- /branches/13/configure.ac https://reviewboard.asterisk.org/r/4546/#comment25549 Added to illustrate that the bits/string2.h is not completely broken and could be used, using a char[]. This extra check should be removed before committing to the repository /branches/13/configure.ac https://reviewboard.asterisk.org/r/4546/#comment25550 This check should be folded into main when test1 is removed. There is no real need to test both strsep and strcmp, they both come down to the same issue in the way __string2_1bptr_p is used bootstrap.sh is needed before committing this to the repository to update configure and include/asterisk/autoconfig.h.in - Diederik de Groot On March 28, 2015, 3:01 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/ --- (Updated March 28, 2015, 3:01 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:-Warray-bounds Diffs - /branches/13/configure.ac 433444 Diff: https://reviewboard.asterisk.org/r/4546/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated March 28, 2015, 3:45 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description (updated) --- 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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:890 enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 use of ARRAY_IN_BOUNDS with enum results in tautological comparison for the lower limit which is always true. Diffs - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4533/ --- (Updated March 28, 2015, 3:43 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:-Wtautological-compare Changes: /branches/13/channels/pjsip/dialplan_functions.c:869 len is of type size_t, which is unsigned. It will not be able to hold a value 0 /branches/13/funcs/func_curl.c:174 Not a 100% sure how to do this correctly. But assiging a negative value is problematic. Extending the enum in curl/curl.h is not possible either. I opted to use the enum last entry (CURL_LAST) which is currently not used for any thing. Another option would be to use one of the OBSOLETE VALUES like 16. Neither way is very nice though. /branches/13/include/asterisk/app.h:988 Needed to convey the error state returned by res/res_stasis_recording.c:stasis_app_recording_if_exists_parse /branches/13/include/asterisk/cel.h:77 Added to convey not-found or error state. Not sure which name would be prefered for such an enum value. /branches/13/main/cel.c:536 Return actual enum instead of -1,l which can not be conveyed by this enum. /branches/13/main/enum.c:260 dn_expand return signed int /branches/13/main/event.c:202 enum type cannot be 0 /branches/13/main/indications.c:362 tone_data.freq1 and freq2 are unsigned int's so no need to check if 0. Not sure what should happend when freq1 / freq2 are 0 already... (needs recheck by source owner) /branches/13/main/presencestate.c:293 Should use the actual enum value for INVALID State /branches/13/main/security_events.c:890 enum event_type cannot be 0 /branches/13/main/udptl.c:365/649/661 encode_length returns and unsigned int, so checking if 0 does not make sence. Not 100% if encode_length has side effects, so left the actual call the this function in place. (Needs to be rechecked by code-owner) /branches/13/res/res_pjsip_exten_state.c:411 Used a temporary int variable to be able to check the return value from ast_hint_presence_state.. Not very nice, but did not want to change the signature of this function. /branches/13/res/res_stasis_playback.c:634 operation is enum and cannot be 0 /branches/13/res/res_stasis_recording.c:598/604 recording-state / operation is enum and cannot be 0 Diffs (updated) - /branches/13/res/res_stasis_recording.c 433444 /branches/13/res/res_stasis_playback.c 433444 /branches/13/res/res_pjsip_exten_state.c 433444 /branches/13/res/ari/resource_channels.c 433444 /branches/13/res/ari/resource_bridges.c 433444 /branches/13/main/udptl.c 433444 /branches/13/main/security_events.c 433444 /branches/13/main/presencestate.c 433444 /branches/13/main/indications.c 433444 /branches/13/main/event.c 433444 /branches/13/main/enum.c 433444 /branches/13/main/cel.c 433444 /branches/13/main/app.c 433444 /branches/13/include/asterisk/utils.h 433444 /branches/13/include/asterisk/cel.h 433444 /branches/13/include/asterisk/app.h 433444 /branches/13/funcs/func_curl.c 433444 /branches/13/channels/pjsip/dialplan_functions.c 433444 Diff: https://reviewboard.asterisk.org/r/4533/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4546: clang compiler warning: -Warray-bounds
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/#review14929 --- /branches/13/channels/pjsip/dialplan_functions.c https://reviewboard.asterisk.org/r/4546/#comment25546 Replacing all the instances of strsep(..., ,) by char delimiter[] = ,; strsep(..., delimiter) as i did in the first iteration of trying to solve this 'warning', would be to big a change (multiple source files affected). The actual issue is cause by strsep macro optimized version of strsep in bits/strings2.h, which expects a character array to be used for the seperator/delimiter and cannot actually deal correctly with a string pointer. New solution - test for this 'errornous' behaviour in configure.ac and prevent strsep being replaced by the _strsep_g from bits/strings2.h if this warning is raised. Do not want to suppress this clang warning completely because it can/could be very usefull in other situations. If this issue get's resolved later on strings2.h we should be able to detect it and use the optimized macro version instead. - Diederik de Groot On March 27, 2015, 6:59 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/ --- (Updated March 27, 2015, 6:59 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:-Warray-bounds Diffs - /branches/13/channels/pjsip/dialplan_functions.c 433444 Diff: https://reviewboard.asterisk.org/r/4546/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4546: clang compiler warning: -Warray-bounds
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/ --- (Updated March 28, 2015, 12:13 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:-Warray-bounds Diffs (updated) - /branches/13/configure.ac 433444 Diff: https://reviewboard.asterisk.org/r/4546/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4546: clang compiler warning: -Warray-bounds
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/#review14933 --- /branches/13/configure.ac https://reviewboard.asterisk.org/r/4546/#comment25547 Test is not working as i hoped yet. Tests inside configure don't seem to get optimized to the same level (-O3), making this test somewhat useless. Trying to find out how to get the same behaviour as wenn running make. - Diederik de Groot On March 28, 2015, 12:14 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/ --- (Updated March 28, 2015, 12:14 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:-Warray-bounds Diffs - /branches/13/configure.ac 433444 Diff: https://reviewboard.asterisk.org/r/4546/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4546: clang compiler warning: -Warray-bounds
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4546/ --- (Updated March 28, 2015, 12:14 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:-Warray-bounds Diffs (updated) - /branches/13/configure.ac 433444 Diff: https://reviewboard.asterisk.org/r/4546/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4527: clang compiler warning: -Wunused-function
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4527/ --- (Updated March 28, 2015, 7:18 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433678 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-function Changes: channels/chan_iax2.c:2026 removed user_ref function because it was not used anywhere main/dsp.c:342 removed goertzel_update function because it was not used anywhere Diffs - /branches/13/main/dsp.c 433444 /branches/13/main/config.c 433470 /branches/13/channels/chan_iax2.c 433444 Diff: https://reviewboard.asterisk.org/r/4527/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4537: clang compiler warning: -Wbitfield-constant-conversion
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4537/ --- (Updated March 28, 2015, 7:31 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433683 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:-Wbitfield-constant-conversion Changes: chan_iax2.c:11898:18: warning: implicit truncation from 'int' to bitfield changes value from -1 to 1 [-Wbitfield-constant-conversion] fr-outoforder = -1; Changed to fr-outoforder = 1; Diffs - /branches/13/channels/chan_iax2.c 433444 Diff: https://reviewboard.asterisk.org/r/4537/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4544: clang compiler warning: -Wself-assign
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4544/ --- (Updated March 28, 2015, 7:48 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433691 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:-Wself-assign Diffs - /branches/13/formats/format_wav_gsm.c 433444 /branches/13/formats/format_wav.c 433444 Diff: https://reviewboard.asterisk.org/r/4544/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
