| Message ID | 20250203-inline-funk-v1-1-2f48418e5874@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] docs: netdev: Document guidance on inline functions | expand |
Simon Horman <horms@kernel.org> writes: > Document preference for non inline functions in .c files. > This has been the preference for as long as I can recall > and I was recently surprised to discover that it is undocumented. > > Reported-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> > Closes: https://lore.kernel.org/all/9662e6fe-cc91-4258-aba1-ab5b016a041a@orange.com/ > Signed-off-by: Simon Horman <horms@kernel.org> > --- > Documentation/process/maintainer-netdev.rst | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst > index e497729525d5..1fbb8178b8cd 100644 > --- a/Documentation/process/maintainer-netdev.rst > +++ b/Documentation/process/maintainer-netdev.rst > @@ -408,6 +408,17 @@ at a greater cost than the value of such clean-ups. > > Conversely, spelling and grammar fixes are not discouraged. > > +Inline functions > +---------------- > + > +The use of static inline functions in .c file is strongly discouraged > +unless there is a demonstrable reason for them, usually performance > +related. Rather, it is preferred to omit the inline keyword and allow the > +compiler to inline them as it sees fit. > + > +This is a stricter requirement than that of the general Linux Kernel > +:ref:`Coding Style<codingstyle>` I have no objection to this change, but I do wonder if it does indeed belong in the central coding-style document. I don't think anybody encourages use of "inline" these days...? jon
> Conversely, spelling and grammar fixes are not discouraged. > > +Inline functions > +---------------- > + > +The use of static inline functions in .c file is strongly discouraged I don't think 'static' is relevant here. They probably are static, if they are inline, and to avoid warnings about missing declarations. But we just prefer not to have any sort of inline functions without good justifications within a .c file. A nit pick, so: Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi Simon, Another nit: On 2/3/25 5:59 AM, Simon Horman wrote: > Document preference for non inline functions in .c files. > This has been the preference for as long as I can recall > and I was recently surprised to discover that it is undocumented. > > Reported-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> > Closes: https://lore.kernel.org/all/9662e6fe-cc91-4258-aba1-ab5b016a041a@orange.com/ > Signed-off-by: Simon Horman <horms@kernel.org> > --- > Documentation/process/maintainer-netdev.rst | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst > index e497729525d5..1fbb8178b8cd 100644 > --- a/Documentation/process/maintainer-netdev.rst > +++ b/Documentation/process/maintainer-netdev.rst > @@ -408,6 +408,17 @@ at a greater cost than the value of such clean-ups. > > Conversely, spelling and grammar fixes are not discouraged. > > +Inline functions > +---------------- > + > +The use of static inline functions in .c file is strongly discouraged > +unless there is a demonstrable reason for them, usually performance > +related. Rather, it is preferred to omit the inline keyword and allow the > +compiler to inline them as it sees fit. > + > +This is a stricter requirement than that of the general Linux Kernel > +:ref:`Coding Style<codingstyle>` Is there an ending period (full stop) after that sentence? Could/should there be? Thanks. > + > Resending after review > ~~~~~~~~~~~~~~~~~~~~~~ > > >
Em Mon, 03 Feb 2025 08:00:56 -0700 Jonathan Corbet <corbet@lwn.net> escreveu: > Simon Horman <horms@kernel.org> writes: > > > Document preference for non inline functions in .c files. > > This has been the preference for as long as I can recall > > and I was recently surprised to discover that it is undocumented. > > > > Reported-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> > > Closes: https://lore.kernel.org/all/9662e6fe-cc91-4258-aba1-ab5b016a041a@orange.com/ > > Signed-off-by: Simon Horman <horms@kernel.org> > > --- > > Documentation/process/maintainer-netdev.rst | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst > > index e497729525d5..1fbb8178b8cd 100644 > > --- a/Documentation/process/maintainer-netdev.rst > > +++ b/Documentation/process/maintainer-netdev.rst > > @@ -408,6 +408,17 @@ at a greater cost than the value of such clean-ups. > > > > Conversely, spelling and grammar fixes are not discouraged. > > > > +Inline functions > > +---------------- > > + > > +The use of static inline functions in .c file is strongly discouraged > > +unless there is a demonstrable reason for them, usually performance > > +related. Rather, it is preferred to omit the inline keyword and allow the > > +compiler to inline them as it sees fit. You should probably point to chapter (12) of Documentation/process/coding-style.rst where it mentions that inline for function prototypes and as a way to replace macros are OK. > > + > > +This is a stricter requirement than that of the general Linux Kernel > > +:ref:`Coding Style<codingstyle>` > > I have no objection to this change, but I do wonder if it does indeed > belong in the central coding-style document. I don't think anybody > encourages use of "inline" these days...? Indeed IMO this belongs to the coding style. I would place it close to chapter (12) at Documentation/process/coding-style.rst. Regards, Thanks, Mauro
Em Mon, 3 Feb 2025 20:50:39 +0100 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > Em Mon, 03 Feb 2025 08:00:56 -0700 > Jonathan Corbet <corbet@lwn.net> escreveu: > > > Simon Horman <horms@kernel.org> writes: > > > > > Document preference for non inline functions in .c files. > > > This has been the preference for as long as I can recall > > > and I was recently surprised to discover that it is undocumented. > > > > > > Reported-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> > > > Closes: https://lore.kernel.org/all/9662e6fe-cc91-4258-aba1-ab5b016a041a@orange.com/ > > > Signed-off-by: Simon Horman <horms@kernel.org> > > > --- > > > Documentation/process/maintainer-netdev.rst | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst > > > index e497729525d5..1fbb8178b8cd 100644 > > > --- a/Documentation/process/maintainer-netdev.rst > > > +++ b/Documentation/process/maintainer-netdev.rst > > > @@ -408,6 +408,17 @@ at a greater cost than the value of such clean-ups. > > > > > > Conversely, spelling and grammar fixes are not discouraged. > > > > > > +Inline functions > > > +---------------- > > > + > > > +The use of static inline functions in .c file is strongly discouraged > > > +unless there is a demonstrable reason for them, usually performance > > > +related. Rather, it is preferred to omit the inline keyword and allow the > > > +compiler to inline them as it sees fit. > > You should probably point to chapter (12) of Documentation/process/coding-style.rst > where it mentions that inline for function prototypes and as a way to > replace macros are OK. Heh, I hit enter too quickly... I mean: "inline for function prototypes and as a way to replace macros on header files (*.h) are OK." > > > > + > > > +This is a stricter requirement than that of the general Linux Kernel > > > +:ref:`Coding Style<codingstyle>` > > > > I have no objection to this change, but I do wonder if it does indeed > > belong in the central coding-style document. I don't think anybody > > encourages use of "inline" these days...? > > Indeed IMO this belongs to the coding style. I would place it close > to chapter (12) at Documentation/process/coding-style.rst. > > Regards, > > Thanks, > Mauro Thanks, Mauro
On Mon, Feb 03, 2025 at 04:10:24PM +0100, Andrew Lunn wrote: > > Conversely, spelling and grammar fixes are not discouraged. > > > > +Inline functions > > +---------------- > > + > > +The use of static inline functions in .c file is strongly discouraged > > I don't think 'static' is relevant here. They probably are static, if > they are inline, and to avoid warnings about missing declarations. But > we just prefer not to have any sort of inline functions without good > justifications within a .c file. > > A nit pick, so: > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Thanks Andrew, I agree that static is not helpful here, I'll drop that in a v2.
On Mon, Feb 03, 2025 at 08:50:39PM +0100, Mauro Carvalho Chehab wrote: > Em Mon, 03 Feb 2025 08:00:56 -0700 > Jonathan Corbet <corbet@lwn.net> escreveu: > > > Simon Horman <horms@kernel.org> writes: > > > > > Document preference for non inline functions in .c files. > > > This has been the preference for as long as I can recall > > > and I was recently surprised to discover that it is undocumented. > > > > > > Reported-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> > > > Closes: https://lore.kernel.org/all/9662e6fe-cc91-4258-aba1-ab5b016a041a@orange.com/ > > > Signed-off-by: Simon Horman <horms@kernel.org> > > > --- > > > Documentation/process/maintainer-netdev.rst | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst > > > index e497729525d5..1fbb8178b8cd 100644 > > > --- a/Documentation/process/maintainer-netdev.rst > > > +++ b/Documentation/process/maintainer-netdev.rst > > > @@ -408,6 +408,17 @@ at a greater cost than the value of such clean-ups. > > > > > > Conversely, spelling and grammar fixes are not discouraged. > > > > > > +Inline functions > > > +---------------- > > > + > > > +The use of static inline functions in .c file is strongly discouraged As suggested by Andrew Lunn elsewhere in this thread I will drop "static" from the line above. > > > +unless there is a demonstrable reason for them, usually performance > > > +related. Rather, it is preferred to omit the inline keyword and allow the > > > +compiler to inline them as it sees fit. > > You should probably point to chapter (12) of Documentation/process/coding-style.rst > where it mentions that inline for function prototypes and as a way to >static replace macros are OK. Thanks, perhaps something like this would help: Using inline in .h files is fine and is encouraged in place of macros [reference section 12]. > > > > + > > > +This is a stricter requirement than that of the general Linux Kernel > > > +:ref:`Coding Style<codingstyle>` > > > > I have no objection to this change, but I do wonder if it does indeed > > belong in the central coding-style document. I don't think anybody > > encourages use of "inline" these days...? > > Indeed IMO this belongs to the coding style. I would place it close > to chapter (12) at Documentation/process/coding-style.rst. Sure, thanks to you and Jonathan for the positive feedback there. I will prepare a revised patch that updates coding-style.rst instead.
On Mon, Feb 03, 2025 at 08:53:12PM +0100, Mauro Carvalho Chehab wrote: > Em Mon, 3 Feb 2025 20:50:39 +0100 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > > > Em Mon, 03 Feb 2025 08:00:56 -0700 > > Jonathan Corbet <corbet@lwn.net> escreveu: > > > > > Simon Horman <horms@kernel.org> writes: > > > > > > > Document preference for non inline functions in .c files. > > > > This has been the preference for as long as I can recall > > > > and I was recently surprised to discover that it is undocumented. > > > > > > > > Reported-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> > > > > Closes: https://lore.kernel.org/all/9662e6fe-cc91-4258-aba1-ab5b016a041a@orange.com/ > > > > Signed-off-by: Simon Horman <horms@kernel.org> > > > > --- > > > > Documentation/process/maintainer-netdev.rst | 11 +++++++++++ > > > > 1 file changed, 11 insertions(+) > > > > > > > > diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst > > > > index e497729525d5..1fbb8178b8cd 100644 > > > > --- a/Documentation/process/maintainer-netdev.rst > > > > +++ b/Documentation/process/maintainer-netdev.rst > > > > @@ -408,6 +408,17 @@ at a greater cost than the value of such clean-ups. > > > > > > > > Conversely, spelling and grammar fixes are not discouraged. > > > > > > > > +Inline functions > > > > +---------------- > > > > + > > > > +The use of static inline functions in .c file is strongly discouraged > > > > +unless there is a demonstrable reason for them, usually performance > > > > +related. Rather, it is preferred to omit the inline keyword and allow the > > > > +compiler to inline them as it sees fit. > > > > You should probably point to chapter (12) of Documentation/process/coding-style.rst > > where it mentions that inline for function prototypes and as a way to > > replace macros are OK. > > Heh, I hit enter too quickly... > > I mean: > "inline for function prototypes and as a way to replace macros on > header files (*.h) are OK." Likewise, I responded to your previous message too quickly. Yes, I agree something like that would be good. > > > > > > > + > > > > +This is a stricter requirement than that of the general Linux Kernel > > > > +:ref:`Coding Style<codingstyle>` > > > > > > I have no objection to this change, but I do wonder if it does indeed > > > belong in the central coding-style document. I don't think anybody > > > encourages use of "inline" these days...? > > > > Indeed IMO this belongs to the coding style. I would place it close > > to chapter (12) at Documentation/process/coding-style.rst. > > > > Regards, > > > > Thanks, > > Mauro > > > > Thanks, > Mauro >
On Mon, Feb 03, 2025 at 10:51:49AM -0800, Randy Dunlap wrote: > Hi Simon, > > Another nit: > > On 2/3/25 5:59 AM, Simon Horman wrote: > > Document preference for non inline functions in .c files. > > This has been the preference for as long as I can recall > > and I was recently surprised to discover that it is undocumented. > > > > Reported-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> > > Closes: https://lore.kernel.org/all/9662e6fe-cc91-4258-aba1-ab5b016a041a@orange.com/ > > Signed-off-by: Simon Horman <horms@kernel.org> > > --- > > Documentation/process/maintainer-netdev.rst | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst > > index e497729525d5..1fbb8178b8cd 100644 > > --- a/Documentation/process/maintainer-netdev.rst > > +++ b/Documentation/process/maintainer-netdev.rst > > @@ -408,6 +408,17 @@ at a greater cost than the value of such clean-ups. > > > > Conversely, spelling and grammar fixes are not discouraged. > > > > +Inline functions > > +---------------- > > + > > +The use of static inline functions in .c file is strongly discouraged > > +unless there is a demonstrable reason for them, usually performance > > +related. Rather, it is preferred to omit the inline keyword and allow the > > +compiler to inline them as it sees fit. > > + > > +This is a stricter requirement than that of the general Linux Kernel > > +:ref:`Coding Style<codingstyle>` > > Is there an ending period (full stop) after that sentence? > Could/should there be? Thanks, I think so. I will add one.
> Thanks, perhaps something like this would help: > > Using inline in .h files is fine and is encouraged in place of macros > [reference section 12]. The other major use of them in headers is for stub functions when an API implementation has a Kconfig option. The question is, do we really want to start creating such a list, and have people wanting to add to it? Andrew
On Tue, Feb 04, 2025 at 02:25:07PM +0100, Andrew Lunn wrote: > > Thanks, perhaps something like this would help: > > > > Using inline in .h files is fine and is encouraged in place of macros > > [reference section 12]. > > The other major use of them in headers is for stub functions when an > API implementation has a Kconfig option. The question is, do we really > want to start creating such a list, and have people wanting to add to > it? Good point. Maybe it is sufficient to just make the distinction between .c and .h files.
... > > +Inline functions > > +---------------- > > + > > +The use of static inline functions in .c file is strongly discouraged > > +unless there is a demonstrable reason for them, usually performance > > +related. Rather, it is preferred to omit the inline keyword and allow the > > +compiler to inline them as it sees fit. > > + > > +This is a stricter requirement than that of the general Linux Kernel > > +:ref:`Coding Style<codingstyle>` > > I have no objection to this change, but I do wonder if it does indeed > belong in the central coding-style document. I don't think anybody > encourages use of "inline" these days...? Apart from the cases where the compiler really ought to inline something but fails to do so because it doesn't notice just how much code collapses out. But in that case you need always_inline. For instance get_sigset_argpack (fs/select.c) is marked inline but isn't being inlined by gcc 12.2 (clang 18 is inlining it). I've also seen places where #defines generate much better code than inline functions because they get processed much earlier. David
diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst index e497729525d5..1fbb8178b8cd 100644 --- a/Documentation/process/maintainer-netdev.rst +++ b/Documentation/process/maintainer-netdev.rst @@ -408,6 +408,17 @@ at a greater cost than the value of such clean-ups. Conversely, spelling and grammar fixes are not discouraged. +Inline functions +---------------- + +The use of static inline functions in .c file is strongly discouraged +unless there is a demonstrable reason for them, usually performance +related. Rather, it is preferred to omit the inline keyword and allow the +compiler to inline them as it sees fit. + +This is a stricter requirement than that of the general Linux Kernel +:ref:`Coding Style<codingstyle>` + Resending after review ~~~~~~~~~~~~~~~~~~~~~~
Document preference for non inline functions in .c files. This has been the preference for as long as I can recall and I was recently surprised to discover that it is undocumented. Reported-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> Closes: https://lore.kernel.org/all/9662e6fe-cc91-4258-aba1-ab5b016a041a@orange.com/ Signed-off-by: Simon Horman <horms@kernel.org> --- Documentation/process/maintainer-netdev.rst | 11 +++++++++++ 1 file changed, 11 insertions(+)