nuttx icon indicating copy to clipboard operation
nuttx copied to clipboard

inline: switch from inline to inline_function

Open pkarashchenko opened this issue 2 years ago • 8 comments

Summary

inline keyword is a C99 extension, so inline functions must be treated based of compiler.h selection.

Impact

Testing

Draft changes

pkarashchenko avatar Jan 10 '22 20:01 pkarashchenko

How about applying this change only to the static inline functions from header files and simply removing the inline keyword from functions defined in source/implementation files?

Usually the compiler is smart enough to decide whether a function should be inlined and the always_inline attribute may induce an unintended increase on code size.

gustavonihei avatar Jan 10 '22 21:01 gustavonihei

How about applying this change only to the static inline functions from header files and simply removing the inline keyword from functions defined in source/implementation files?

Usually the compiler is smart enough to decide whether a function should be inlined and the always_inline attribute may induce an unintended increase on code size.

Or as an alternative we can introduce inline_function and forceinline_function so that can be selectable in header and source code files.

pkarashchenko avatar Jan 10 '22 21:01 pkarashchenko

How about applying this change only to the static inline functions from header files and simply removing the inline keyword from functions defined in source/implementation files? Usually the compiler is smart enough to decide whether a function should be inlined and the always_inline attribute may induce an unintended increase on code size.

Or as an alternative we can introduce inline_function and forceinline_function so that can be selectable in header and source code files.

Sorry, maybe I understood it incorrectly, but would both macros be defined to the same always_inline attribute? If that is the case, this will end up confusing. I would agree with renaming the inline_function macro to forceinline_function, makes it more intuitive this way. Unfortunately, afaik, there is no function attribute to 1:1 substitute the inline keyword, to just give a hint to the compiler. So, for these cases, I think we'd better just trust the compiler's optimization passes to perform the necessary inlining.

gustavonihei avatar Jan 10 '22 22:01 gustavonihei

How about applying this change only to the static inline functions from header files and simply removing the inline keyword from functions defined in source/implementation files? Usually the compiler is smart enough to decide whether a function should be inlined and the always_inline attribute may induce an unintended increase on code size.

Or as an alternative we can introduce inline_function and forceinline_function so that can be selectable in header and source code files.

Sorry, maybe I understood it incorrectly, but would both macros be defined to the same always_inline attribute? If that is the case, this will end up confusing. I would agree with renaming the inline_function macro to forceinline_function, makes it more intuitive this way. Unfortunately, afaik, there is no function attribute to 1:1 substitute the inline keyword, to just give a hint to the compiler. So, for these cases, I think we'd better just trust the compiler's optimization passes to perform the necessary inlining.

Currently in code there are two qualifiers used for inline functions:

  1. inline -- the C99 extension
  2. inline_funtion -- that basically is a "force inline" directive to compilers that support it.

We need common code to be C89 compatible, so most probably need to replace both cases with macro definitions. For now I do not know compilers that support force inlining but do not support inline keyword one or another way (I mean that some compilers support __inline instead of inline but with the same meaning).

In this initial change I start replacing inline with inline_function (that is currently a force inline actually) so now I'm thinking of:

  1. Rework inline_function to be replacement for inline
  2. Introduce new force_inline to handle force inline case (the current inline_function).

pkarashchenko avatar Jan 10 '22 22:01 pkarashchenko

In this initial change I start replacing inline with inline_function (that is currently a force inline actually) so now I'm thinking of:

  1. Rework inline_function to be replacement for inline

My suggestion for removing some of the inline occurrences fits here. Those inline functions that live in .c files could have the keyword simply removed. Unless there is a compiler flag that hints the compiler to inline the function, I believe the least intrusive approach is to do nothing and trust the compiler:

#define inline
  1. Introduce new force_inline to handle force inline case (the current inline_function).

And here comes my previous suggestion for just renaming the inline_function into forceinline_function, and adding forceinline_function to the functions defined in header files which are expected to be inlined. So this way we would be compliant to the real intent of the attribute, which is to indeed force the compiler to inline the function.

gustavonihei avatar Jan 10 '22 22:01 gustavonihei

In this initial change I start replacing inline with inline_function (that is currently a force inline actually) so now I'm thinking of:

  1. Rework inline_function to be replacement for inline
  2. Introduce new force_inline to handle force inline case (the current inline_function).

This makes sense to me.

inline_function means "hint the compiler to inline this function, but compiler may decide not to"

force_inline_function means "tell the compiler it must inline this function"

If we discover a compiler that supports inline_function but not force_inline_function then on that compiler we will "degrade gracefully" and force_inline_function will mean the same thing as inline_function.

hartmannathan avatar Jan 11 '22 02:01 hartmannathan

@pkarashchenko please fix the coding style of "include/nuttx/tree.h" before apply this commit, seams like the original file was already with coding style issues

acassis avatar Jan 11 '22 13:01 acassis

@pkarashchenko please fix the coding style of "include/nuttx/tree.h" before apply this commit, seams like the original file was already with coding style issues

Yes. I will fix style issue. This is currently a draft PR just to agree on idea in general how to proceed.

pkarashchenko avatar Jan 11 '22 13:01 pkarashchenko