[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