<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/4550/">https://reviewboard.asterisk.org/r/4550/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 31st, 2015, 7:14 p.m. CEST, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On March 31st, 2015, 8:30 p.m. CEST, <b>Diederik de Groot</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On March 31st, 2015, 8:31 p.m. CEST, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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");
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">@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.
</pre>
<br />
<p>- Diederik</p>
<br />
<p>On March 29th, 2015, 7:14 p.m. CEST, Diederik de Groot wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Diederik de Groot.</div>
<p style="color: grey;"><i>Updated March 29, 2015, 7:14 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-24917">ASTERISK-24917</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/13/main/stdtime/localtime.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/main/stasis_message_router.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/main/format_cap.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/main/astobj2_hash.c <span style="color: grey">(433444)</span></li>
<li>/branches/13/include/asterisk/vector.h <span style="color: grey">(433444)</span></li>
<li>/branches/13/include/asterisk/linkedlists.h <span style="color: grey">(433444)</span></li>
<li>/branches/13/include/asterisk/dlinkedlists.h <span style="color: grey">(433444)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4550/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>