You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/10 20:30:20 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request #5201: inline: switch from inline to inline_function
pkarashchenko opened a new pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201
## Summary
`inline` keyword is a C99 extension, so inline functions must be treated based of `compiler.h` selection.
## Impact
## Testing
Draft changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009551995
> 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#discussion_r788044486
##########
File path: arch/arm/include/armv6-m/irq.h
##########
@@ -219,8 +219,7 @@ struct xcptcontext
/* Get/set the PRIMASK register */
-static inline uint8_t getprimask(void) inline_function;
-static inline uint8_t getprimask(void)
+inline_function static uint8_t getprimask(void)
Review comment:
> @a-lunev thanks for letting me know... do you agree that we should use this in the precheck builds (or maybe even in all builds, to catch standard violations as soon as they occur)?
Yes, I like this idea for automatic validation by passing -std=c89 option. Though, I do not know what the stage is better (pre-check or a standard part of the build system). I am not yet familiar to all internals how the NuttX build system works. Is the pre-check invoked every time I run "make" command to build NuttX?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009413044
> 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:
```c
#define inline
```
> 2. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#discussion_r787987735
##########
File path: arch/arm/include/armv6-m/irq.h
##########
@@ -219,8 +219,7 @@ struct xcptcontext
/* Get/set the PRIMASK register */
-static inline uint8_t getprimask(void) inline_function;
-static inline uint8_t getprimask(void)
+inline_function static uint8_t getprimask(void)
Review comment:
> I think it would be a good thing to pass -std=c89 on common files in the precheck. Then if someone inadvertently uses C99 in common files, it will be noticed immediately.
@hartmannathan I have just tested -std=c89 option to check if gcc detects errors in case of mixed declarations and code.
As it turned out, -std=c89 is not enough. Only "-std=c89 -Wpedantic" detects this case:
```
$ gcc -x c -std=c89 -Wpedantic - <<EOF
> int main() {
> int a;
> a = 2;
> int b;
> return 0;
> }
> EOF
<stdin>: In function ‘main’:
<stdin>:4:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009976604
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009356039
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009386218
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#discussion_r781716665
##########
File path: include/nuttx/compiler.h
##########
@@ -200,11 +200,11 @@
# define __syslog__ __printf__
# endif
-# define formatlike(a) __attribute__((__format_arg__ (a)))
-# define printflike(a, b) __attribute__((__format__ (__printf__, a, b)))
-# define sysloglike(a, b) __attribute__((__format__ (__syslog__, a, b)))
-# define scanflike(a, b) __attribute__((__format__ (__scanf__, a, b)))
-# define strftimelike(a) __attribute__((__format__ (__strftime__, a, 0)))
+# define formatlike(a) __attribute__ ((__format_arg__ (a)))
Review comment:
should we keep the old style(no space before '(')?
##########
File path: include/nuttx/compiler.h
##########
@@ -182,14 +182,14 @@
# if defined(__clang__)
# define nostackprotect_function __attribute__ ((optnone))
# else
-# define nostackprotect_function __attribute__ ((__optimize__ ("-fno-stack-protector")))
+# define nostackprotect_function __attribute__ ((optimize ("-fno-stack-protector")))
Review comment:
remove the space before '('?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009356039
How about applying this change only to the `static inline` functions from header files and simply removing the `inline` from 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009965058
@pkarashchenko please fix the coding style of "include/nuttx/tree.h" before apply this commit, since like the original file was already with coding style issues
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009386218
> > 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 substiture 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on a change in pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#discussion_r782311674
##########
File path: arch/arm/include/armv6-m/irq.h
##########
@@ -219,8 +219,7 @@ struct xcptcontext
/* Get/set the PRIMASK register */
-static inline uint8_t getprimask(void) inline_function;
-static inline uint8_t getprimask(void)
+inline_function static uint8_t getprimask(void)
Review comment:
I think it would be a good thing to pass -std=c89 on common files in the precheck. Then if someone inadvertently uses C99 in common files, it will be noticed immediately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on a change in pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#discussion_r788034978
##########
File path: arch/arm/include/armv6-m/irq.h
##########
@@ -219,8 +219,7 @@ struct xcptcontext
/* Get/set the PRIMASK register */
-static inline uint8_t getprimask(void) inline_function;
-static inline uint8_t getprimask(void)
+inline_function static uint8_t getprimask(void)
Review comment:
@a-lunev thanks for letting me know... do you agree that we should use this in the precheck builds (or maybe even in all builds, to catch standard violations as soon as they occur)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009356039
How about applying this change only to the `static inline` functions from header files and simply removing the `inline` keyword from functions defined in 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#discussion_r782161109
##########
File path: arch/arm/include/armv6-m/irq.h
##########
@@ -219,8 +219,7 @@ struct xcptcontext
/* Get/set the PRIMASK register */
-static inline uint8_t getprimask(void) inline_function;
-static inline uint8_t getprimask(void)
+inline_function static uint8_t getprimask(void)
Review comment:
I noticed that some functions like this one had "inline" before the name of the function and "inline_function" at the end of the function. It is really strange. Maybe we can try to pass "-std=c89" for some arch that is already following C89 to try to catch these issues. Other option is to find "an old era" C89 compiler for some retro arch and test it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009395485
> > > 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`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009379846
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis edited a comment on pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
acassis edited a comment on pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#issuecomment-1009965058
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on a change in pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#discussion_r782312406
##########
File path: arch/arm/include/armv6-m/irq.h
##########
@@ -219,8 +219,7 @@ struct xcptcontext
/* Get/set the PRIMASK register */
-static inline uint8_t getprimask(void) inline_function;
-static inline uint8_t getprimask(void)
+inline_function static uint8_t getprimask(void)
Review comment:
(Maybe not just in the precheck, but as a standard part of the build system?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5201: inline: switch from inline to inline_function
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5201:
URL: https://github.com/apache/incubator-nuttx/pull/5201#discussion_r787950643
##########
File path: arch/arm/include/armv6-m/irq.h
##########
@@ -219,8 +219,7 @@ struct xcptcontext
/* Get/set the PRIMASK register */
-static inline uint8_t getprimask(void) inline_function;
-static inline uint8_t getprimask(void)
+inline_function static uint8_t getprimask(void)
Review comment:
Yes, I think passing -std=c89 for common files is a good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org