[asterisk-dev] [Code Review] 4550: clang compiler warning: --dev-mode and -Wparentheses-equality

[Diederik de Groot]

> 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 
> 
> Diff: https://reviewboard.asterisk.org/r/4550/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Diederik de Groot
> 
>

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