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

Diederik de Groot reviewboard at asterisk.org
Tue Apr 7 20:06:56 CDT 2015 


> 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.
> 
> Diederik de Groot wrote:
>     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.
> 
> rmudgett wrote:
>     I don't think that value can ever get passed to the curl library.  The value only seems to be used for configuration.  The curl library won't know what to do with it anyway.

Didn't look too deep into it to be honest. Taking your word for it ;-)


- Diederik


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


On April 8, 2015, 1:26 a.m., Diederik de Groot wrote:
> 
> -----------------------------------------------------------
> 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
> -------
> 
> Compiles without warning
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

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

More information about the asterisk-dev mailing list