[asterisk-dev] [Code Review] 4533: clang compiler warning: -Wtautological-compare

[Diederik de Groot]

> On March 26, 2015, 11:13 p.m., Diederik de Groot wrote:
> > /branches/13/channels/pjsip/dialplan_functions.c, line 869
> > <https://reviewboard.asterisk.org/r/4533/diff/1/?file=72956#file72956line869>
> >
> >     len is of type size_t, which is unsigned. It will not be able to hold a value < 0
> 
> Matt Jordan wrote:
>     Rather than opening issues against your review - which indicate that there is some problem with the code change - I would instead put these kinds of descriptions into your description of the code change.
>     
>     That's helpful as well for when the change is merged - someone has to craft a commit message that describes the code change, and having a well written description that describes each change is almost always better than a terse sentence :-)
>     
>     As it is, make sure you close out the "issues" so that reviewers know that these aren't actually problems.

Thanks for the suggestion, still trying to figure out the best way to pass all these changes along. Have dropped all the 'issues' and added them to the description above.


- Diederik


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


On March 27, 2015, 11:42 a.m., Diederik de Groot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4533/
> -----------------------------------------------------------
> 
> (Updated March 27, 2015, 11: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
> 
> 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
> -----
> 
>   /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/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
> 
>

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