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? > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
