[Home]

Summary:ASTERISK-04487: [patch] simplify macros for inline/not inline API functions
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-06-26 12:50:26Date Closed:2008-01-15 15:40:49.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) ast_header
Description:The recent changes to put inlinable API functions only in one place
is nice but still a bit too convoluted as it requires to write
the prototype twice, and has a lot if #ifdef... clutter.
The attached file provides a simplified way to write these functions,
embeded in a macro F(header, body) which is defined appropriately according
to the value of LOW_MEMORY and AST_API_MODULE. See example below.
The #if ... block needs to be written only once in the header file
(possibly just in util.h if we put there the #undef AST_API_MODULE)
then the various API functions are encapsulated in the F() macro.
Only catch is that they cannot have preprocessor directives inside,
but being simple functions this should not be a major problem.

****** ADDITIONAL INFORMATION ******

F( char *ast_skip_blanks(char *str)    ,
{
       while (*str && *str < 33)
               str++;
       return str;
} )
Comments:By: Kevin P. Fleming (kpfleming) 2005-07-11 17:20:33

I have a couple of concerns here (one minor, one major):

- this will wreak havoc on editors that do automatic code indentation, since they will see another layer of 'brackets'

- with my compilers, I can't define a non-static function without previously having declared a prototype for it (otherwise I get a warning similar to "no previous prototype found"); that was the reason for doubling the 'header' portion of the function definition in that case
By: Luigi Rizzo (rizzo) 2005-07-11 17:30:13

can't comment on the auto-indenting editors,
but the non-static without prototype is a bit strange...
what compiler and what compiler flags give that error ?
By: Kevin P. Fleming (kpfleming) 2005-07-11 17:41:52

GCC 3.3.x and GCC 4.0.x with the standard compiler flags in the Makefile produce this warning (it's not an error, just a warning). It's intended to ensure that you don't accidentally make a function exposed when you didn't mean to.
By: Luigi Rizzo (rizzo) 2005-07-11 17:50:47

..hmm then one way could be to have the macro emit both the prototype
and the function in the two cases, as below: (not sure if you need
to replicate any extern and inline on the prototype)

- #define F(hdr, body) hdr body
+ #define F(hdr, body) hdr ; hdr body

On passing, i am not sure if you really need 'extern inline' or
'static inline' is ok for the !LOW_MEMORY !AST_API_MODULE
By: Kevin P. Fleming (kpfleming) 2005-07-11 18:24:23

Yes, 'extern inline' is useful, because in cases where the compiler cannot inline the function (due to size, -O0 or other restrictions), the definition converts into an 'extern', as opposed to 'static'. This means that we don't end up with 10-30 copies of the function body in every module, they all use the common one when it cannot be inlined.

I'll have to play around with these macros and see how they affect editing; it may not be a big enough problem to justify finding some more complex solution.
By: Kevin P. Fleming (kpfleming) 2005-07-11 19:17:51

OK, I've committed a slightly different version, with different documentation. Thanks for the idea, though, it makes the code much easier to read and maintain!
By: Digium Subversion (svnbot) 2008-01-15 15:40:49.000-0600

Repository: asterisk
Revision: 6090

A   trunk/include/asterisk/inline_api.h
U   trunk/include/asterisk/strings.h
U   trunk/include/asterisk/time.h
U   trunk/include/asterisk/utils.h

------------------------------------------------------------------------
r6090 | kpfleming | 2008-01-15 15:40:49 -0600 (Tue, 15 Jan 2008) | 2 lines

simplify (and document!) macro for inlinable API functions (inspired by bug ASTERISK-4487, with slightly different implementation)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6090