[asterisk-dev] [Code Review] 4370: RAII_VAR: nested functions aren't portable. Adapting RAII_VAR to use clang/llvm blocks to get the same/similar functionality. TAKE 2

[Diederik de Groot]

> On Feb. 20, 2015, 6:03 p.m., Corey Farrell wrote:
> > /branches/11/include/asterisk/utils.h, lines 946-948
> > <https://reviewboard.asterisk.org/r/4370/diff/2/?file=71539#file71539line946>
> >
> >     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.
> 
> Matt Jordan wrote:
>     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.
> 
> Corey Farrell wrote:
>     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
> 
> Diederik de Groot wrote:
>     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).
> 
> Matt Jordan wrote:
>     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.

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.


- Diederik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4370/#review14513
-----------------------------------------------------------


On March 12, 2015, 1:27 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4370/
> -----------------------------------------------------------
> 
> (Updated March 12, 2015, 1:27 p.m.)
> 
> 
> Review request for Asterisk Developers and Diederik de Groot.
> 
> 
> Bugs: ASTERISK-20850
>     https://issues.asterisk.org/jira/browse/ASTERISK-20850
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /branches/11/makeopts.in 432053 
>   /branches/11/main/Makefile 432053 
>   /branches/11/include/asterisk/utils.h 432053 
>   /branches/11/include/asterisk/inline_api.h 432053 
>   /branches/11/configure.ac 432053 
>   /branches/11/configure UNKNOWN 
>   /branches/11/Makefile 432053 
> 
> Diff: https://reviewboard.asterisk.org/r/4370/diff/
> 
> 
> Testing
> -------
> 
> * 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.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150326/1a0267da/attachment.html>