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

Diederik de Groot reviewboard at asterisk.org
Tue Mar 31 13:37:54 CDT 2015



> 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
> =====================
> 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/9bb3929a/attachment.html>


More information about the asterisk-dev mailing list