> On March 31, 2015, 12: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.
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");
- rmudgett
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
-----------------------------------------------------------
On March 29, 2015, 12: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, 12: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
>
>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
