<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/4370/">https://reviewboard.asterisk.org/r/4370/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 20th, 2015, 6:03 p.m. CET, <b>Corey Farrell</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/4370/diff/2/?file=71539#file71539line946" style="color: black; font-weight: bold; text-decoration: underline;">/branches/11/include/asterisk/utils.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">char *ast_utils_which(const char *binary, char *fullpath, size_t fullpath_size);</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">946</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp"> </span><span class="cm">/* Prototype needed due to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36774 */</span><span class="cp"> \</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">946</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#if defined(__clang__)</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">947</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp"> auto void _dtor_ ## varname (vartype * v); \</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">947</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">948</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp"> void _dtor_ ## varname (vartype * v) { dtor(*v); } \</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">948</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#if defined(__has_feature) && __has_feature(blocks)</span></pre></td>
</tr>
</tbody>
</table>
<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 feel that configure should create a #define for the type of nested procedure supported. This should use results of configure (#ifdef HAVE_CLANG_BLOCKS), instead of implementing it's own direct tests.</pre>
</blockquote>
<p>On February 20th, 2015, 6:23 p.m. CET, <b>Matt Jordan</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;">Hm. I'm not sure how much better that makes this, since I find parsing out a configure.ac file to be much harder than the #defines in a header.
What specifically are you envisioning? Right now, I'm having a hard time determining how this would look in the configure script and how it would be reflected here.</pre>
</blockquote>
<p>On February 21st, 2015, 7:18 p.m. CET, <b>Corey Farrell</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;">The idea is that configure is responsible for testing features and reporting what was found, so we shouldn't attempt to duplicate the logic of those tests. The tests performed in configure are better anyways.
In configure.ac where it finds working CLANG support, we want to add:
AC_DEFINE([HAVE_CLANG_BLOCKS], 1, [Define to 1 if your compiler supports CLANG blocks.])
And when it finds GCC nested functions:
AC_DEFINE([HAVE_GCC_NESTED_FUNCTIONS], 1, [Define to 1 if your compiler supports GCC nested functions.])
Then when you run bootstrap / configure, you will get the appropriate #define / #undef in asterisk/autoconfig.h.
Then in asterisk/utils.h:
#if defined(HAVE_CLANG_BLOCKS)
/* CLANG support code */
#elif defined(HAVE_GCC_NESTED_FUNCTIONS)
/* GCC support code */
#else
#error
#endif</pre>
</blockquote>
<p>On February 22nd, 2015, 12:35 p.m. CET, <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;">This almost sounds like replacing one define for another. This is not being done for other (gcc) builtin defines/attributes in other parts of the code.
It might also make crosscompiling a lot more difficult, clang might take the destination into account when evaluating the __has_feature() (crosscompilation should be tested/checked, to check what happens). </pre>
</blockquote>
<p>On March 11th, 2015, 11:58 p.m. CET, <b>Matt Jordan</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'm kind of ambivalent about the issue - really, we're just moving around where the ugliness lives.
One issue with moving this into the configure script is that it is difficult to tell whether or not gcc truly supports nested functions. The existing test attempts to compile a snippet to work around a bug in gcc - if that succeeds, then it doesn't bother with the 'fnested-functions' option. If it fails, then it uses that option. In neither case, however, does that imply that nested functions don't work - it is merely working out whether or not it needs the compiler option.
(This took awhile to figure out, which explains my reticence about pushing more into the configure script).
Corey: if you feel strongly about this, then I don't mind trying to move more of this into the configure check, but otherwise, I'd move to leave the ugliness where it lies.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">We are actually experiencing problems with using "#error" in the header file, when using clang to compile an external module against asterisk compiled using gcc. We are not using the RAII feature in this module, so there is no reason to recheck if utils.h was compiled with clang including the blocks feature. The check for the block feature should only be done during configure in asterisk, and should be removed from the utils.h file.</pre>
<br />
<p>- Diederik</p>
<br />
<p>On March 12th, 2015, 1:27 p.m. CET, Matt Jordan 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 and Diederik de Groot.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated March 12, 2015, 1:27 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-20850">ASTERISK-20850</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;">This is a continuation of the patch put up for review on r3488. It addresses the issues found on that review.
This patch *should* make Asterisk compile under clang. Note that compiling with --enable-dev-mode will cause Asterisk to fail to compile under clang, as it detects a number of warnings that aren't fixed under this patch.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">* Compiled Asterisk with and without --enable-dev-mode using gcc. Asterisk compiles correctly.
* Compiled Asterisk without --enable-dev-mode using clang. Asterisk compiles, links, and executes.
Note that you will need the BlocksRuntime to run Asterisk when it is compiled with clang.</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/11/makeopts.in <span style="color: grey">(432053)</span></li>
<li>/branches/11/main/Makefile <span style="color: grey">(432053)</span></li>
<li>/branches/11/include/asterisk/utils.h <span style="color: grey">(432053)</span></li>
<li>/branches/11/include/asterisk/inline_api.h <span style="color: grey">(432053)</span></li>
<li>/branches/11/configure.ac <span style="color: grey">(432053)</span></li>
<li>/branches/11/configure <span style="color: grey">(UNKNOWN)</span></li>
<li>/branches/11/Makefile <span style="color: grey">(432053)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4370/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>