On 13 Jul 2026, at 18:03, Aaron Conole wrote:
> Ilya Maximets <[email protected]> writes: > >> On 7/9/26 1:52 PM, Eelco Chaudron via dev wrote: >>> >>> >>> On 9 Jul 2026, at 13:46, Dmitry Mityugov wrote: >>> >>>> On Wed, Jun 24, 2026 at 5:04 PM Eelco Chaudron via dev >>>> <[email protected]> wrote: >>>>> >>>>> >>>>> >>>>> On 23 Jun 2026, at 23:42, Aaron Conole wrote: >>>>> >>>>>> Eelco Chaudron <[email protected]> writes: >>>>>> >>>>>>> On 23 Jun 2026, at 21:21, Aaron Conole wrote: >>>>>>> >>>>>>>> Modern optimizing compilers will automatically choose to inline when >>>>>>>> it is appropriate for specific functions. In fact, 'inline' is >>>>>>>> usually considered harmful[0] except in specific situations (such >>>>>>>> as using them in header files to prevent linker errors when the >>>>>>>> One Definition Rule needs to be relaxed). >>>>>>>> >>>>>>>> Signed-off-by: Aaron Conole <[email protected]> >>>>>>>> >>>>>>>> 0: >>>>>>>> https://bylizhao.github.io/c/programming/2020/12/12/Static-inline-considered-harmful.html >>>>>>>> --- >>>>>>> >>>>>>> Thanks for sorting this out Aaron! >>>>>> >>>>>> Thanks for the quick turnaround! >>>>>> >>>>>>> See some comments below. >>>>>>> >>>>>>> //Eelco >>>>>>> >>>>>>>> tests/checkpatch.at | 70 +++++++++++++++++++++++++++++++++++++++++ >>>>>>>> utilities/checkpatch.py | 11 ++++--- >>>>>>>> 2 files changed, 77 insertions(+), 4 deletions(-) >>>>>>>> >>>>>>>> diff --git a/tests/checkpatch.at b/tests/checkpatch.at >>>>>>>> index 7d80ffbed5..a1b915fa48 100755 >>>>>>>> --- a/tests/checkpatch.at >>>>>>>> +++ b/tests/checkpatch.at >>>>>>>> @@ -750,6 +750,76 @@ try_checkpatch_c_file \ >>>>>>>> done >>>>>>>> AT_CLEANUP >>>>>>>> >>>>>>>> +AT_SETUP([checkpatch - file contents checks - static inline in C >>>>>>>> files]) >>>>>>>> + >>>>>>>> +try_checkpatch_c_file \ >>>>>>>> + "#include <foo.h> >>>>>>>> + #include <bar.h> >>>>>>>> + >>>>>>>> + static inline bool foo() { >>>>>>>> + }" \ >>>>>>>> + "ERROR: Using 'static inline' in a c-file. >>>>>>>> + test.c:4: >>>>>>>> + static inline bool foo() {" >>>>>>>> + >>>>>>>> +try_checkpatch_c_file \ >>>>>>>> + "#include <foo.h> >>>>>>>> + #include <bar.h> >>>>>>>> + >>>>>>>> + static inline >>>>>>>> + bool foo() { >>>>>>>> + }" \ >>>>>>>> + "ERROR: Using 'static inline' in a c-file. >>>>>>>> + test.c:4: >>>>>>>> + static inline" >>>>>>>> + >>>>>>>> +try_checkpatch_c_file \ >>>>>>>> + "#include <foo.h> >>>>>>>> + #include <bar.h> >>>>>>>> + >>>>>>>> + static inline >>>>>>>> + bool foo() { >>>>>>>> + }" \ >>>>>>>> + "ERROR: Using 'static inline' in a c-file. >>>>>>>> + test.c:4: >>>>>>>> + static inline" >>>>>>>> + >>>>>>>> +try_checkpatch_c_file \ >>>>>>>> + "#include <foo.h> >>>>>>>> + #include <bar.h> >>>>>>>> + >>>>>>>> + static inline >>>>>>>> + bool foo() { >>>>>>>> + }" \ >>>>>>>> + "ERROR: Using 'static inline' in a c-file. >>>>>>>> + test.c:4: >>>>>>>> + static inline" >>>>>>>> + >>>>>>>> +try_checkpatch_c_file \ >>>>>>>> + "#include <foo.h> >>>>>>>> + #include <bar.h> >>>>>>>> + >>>>>>>> + static void inliner() { >>>>>>>> + }" >>>>>>>> + >>>>>>>> +try_checkpatch_c_file \ >>>>>>>> + "#include <foo.h> >>>>>>>> + #include <bar.h> >>>>>>>> + >>>>>>>> + static void inliner() { >>>>>>>> + }" >>>>>>>> + >>>>>>>> +dnl This is to confirm that filename selection works >>>>>>>> +try_checkpatch_py_file \ >>>>>>>> + "#include <foo.h> >>>>>>>> + #include <bar.h> >>>>>>>> + >>>>>>>> + static inline bool foo() { >>>>>>>> + }" >>>>>>>> + >>>>>>> >>>>>>> nit; Lots of tests ;) but if static and inline are split it fails: >>>>>>> >>>>>>> +try_checkpatch_c_file \ >>>>>>> + "#include <foo.h> >>>>>>> + #include <bar.h> >>>>>>> + >>>>>>> + static >>>>>>> + inline >>>>>>> + bool foo() { >>>>>>> + }" \ >>>>>>> + "ERROR: Using 'static inline' in a c-file. >>>>>>> + test.c:4: >>>>>>> + static inline" >>>>>> >>>>>> Yes - unfortunately it is kindof how the check system works for source >>>>>> code. Checks are executed 'line-at-a-time' for parsing states. We >>>>>> would need to write something that would keep additional line state >>>>>> across lines, or change how the check system worked to support this >>>>>> case. I thought about the case a little as well, and if someone submits >>>>>> it to try and defeat the checkpatch alert, our inferior human eyeballs >>>>>> and puny coding spidey-sense will hopefully catch it. It looks jarring. >>>>> >>>>> Ack, yes we can keep it as is, not worth the rework for this corner case. >>>> >>>> By the way, static inline functions are still marked as Ok in the >>>> Style Guide. Shouldn't it be fixed together with this patch? >>> >>> If this is the case, we should probably update the documentation. Aaron >>> WDYT? >> There are also cases where the function is in C file, but it must be inlined. >> In such cases we additionally have ALWAYS_INLINE macro. We have at least one >> case like that in dpcls code for userspace datapath. Checkpatch and the >> style guide should allow for those. > > Such a case is documented in the coding-style guideline at > Documentation/internals/contributing/coding-style.rst:209:: > > Functions in ``.c`` files should not normally be marked ``inline``, > because it does not usually help code generation and it does suppress > compiler warnings about unused functions. (Functions defined in > ``.h`` usually should be marked ``inline``.) > > But reading it, that justification seems a bit weak - since it omits > other important rationale like 'static inlines bloat the binary', 'static > inlines create a maintenance burden when introducing new > call sites' (eg: do we need to uninline the function now?), and we don't > get details on the ALWAYS_INLINE case. We can update that documentation > to at least cover the ALWAYS_INLINE case as part of the checkpatch > update, but if we want to also make a stronger case for not using > inline, that probably should be a separate patch. > > Does it make sense? Does to me. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
