> 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.
>
> Diederik de Groot wrote:
> 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.
I guess we should drop this one. Maybe someone will come up with a nice
solution in the future. There is a open issue on the clang bug report site
about this exact warning (currently without a nice solution).
- Diederik
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4550/#review14987
-----------------------------------------------------------
On April 1, 2015, 3:33 a.m., Diederik de Groot wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4550/
> -----------------------------------------------------------
>
> (Updated April 1, 2015, 3:33 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:--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/tests/test_linkedlists.c 433444
> /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
