| Message ID | 20210930235754.2635912-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] docs: Explain the desired position of function attributes | expand |
On Thu, Sep 30, 2021 at 4:58 PM Kees Cook <keescook@chromium.org> wrote: > > While discussing how to format the addition of various function > attributes, some "unwritten rules" of ordering surfaced[1]. Capture as > close as possible to Linus's preferences for future reference. > > (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and > others that would prefer all attributes live on a separate leading line.) > > [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/ > > Signed-off-by: Kees Cook <keescook@chromium.org> While I appreciate you getting the ball across the finish line (having _any_ documentation to point to in future bikesheds), I can't help but shake the feeling that the chosen policy will harm the ability of existing automated code formatting tools from being able to automate code formatting on the kernel. > --- > v4: > - fix another stray "void"! This is why code needs a compiler... (thx randy) > --- > Documentation/process/coding-style.rst | 30 ++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst > index 42969ab37b34..45b48510f5ec 100644 > --- a/Documentation/process/coding-style.rst > +++ b/Documentation/process/coding-style.rst > @@ -487,6 +487,36 @@ because it is a simple way to add valuable information for the reader. > Do not use the ``extern`` keyword with function prototypes as this makes > lines longer and isn't strictly necessary. > > +When writing a function declarations, please keep the `order of elements regular > +<https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/>`_. > +For example:: > + > + extern __init void * __must_check action(enum magic value, size_t size, u8 count, > + char *fmt, ...) __printf(4, 5) __malloc; > + > +The preferred order of elements for a function prototype is: > + > +- storage class (here, ``extern``, and things like ``static __always_inline`` even though > + ``__always_inline`` is technically an attribute, it is treated like ``inline``) > +- storage class attributes (here, ``__init`` -- i.e. section declarations, but also things like ``__cold``) > +- return type (here, ``void *``) > +- return type attributes (here, ``__must_check``) > +- function name (here, ``action``) > +- function parameters (here, ``(enum magic value, size_t size, u8 count, char *fmt, ...)``, noting that parameter names should always be included) > +- function parameter attributes (here, ``__printf(4, 5)``) > +- function behavior attributes (here, ``__malloc``) > + > +Note that for a function definition (e.g. ``static inline``), the compiler does > +not allow function parameter attributes after the function parameters. In these > +cases, they should go after the storage class attributes (e.g. note the changed > +position of ``__printf(4, 5)``):: > + > + static __always_inline __init __printf(4, 5) void * __must_check action(enum magic value, > + size_t size, u8 count, char *fmt, ...) > + __malloc > + { > + ... > + } > > 7) Centralized exiting of functions > ----------------------------------- > -- > 2.30.2 >
On Fri, 2021-10-01 at 12:05 -0700, Nick Desaulniers wrote: > On Thu, Sep 30, 2021 at 4:58 PM Kees Cook <keescook@chromium.org> wrote: > > > > While discussing how to format the addition of various function > > attributes, some "unwritten rules" of ordering surfaced[1]. Capture as > > close as possible to Linus's preferences for future reference. > > > > (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and > > others that would prefer all attributes live on a separate leading line.) > > > > [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/ > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > While I appreciate you getting the ball across the finish line (having > _any_ documentation to point to in future bikesheds), I can't help but > shake the feeling that the chosen policy will harm the ability of > existing automated code formatting tools from being able to automate > code formatting on the kernel. yup.
On Fri, Oct 01, 2021 at 12:05:25PM -0700, Nick Desaulniers wrote: > On Thu, Sep 30, 2021 at 4:58 PM Kees Cook <keescook@chromium.org> wrote: > > > > While discussing how to format the addition of various function > > attributes, some "unwritten rules" of ordering surfaced[1]. Capture as > > close as possible to Linus's preferences for future reference. > > > > (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and > > others that would prefer all attributes live on a separate leading line.) > > > > [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/ > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > While I appreciate you getting the ball across the finish line (having > _any_ documentation to point to in future bikesheds), I can't help but > shake the feeling that the chosen policy will harm the ability of > existing automated code formatting tools from being able to automate > code formatting on the kernel. I am but the messenger here. Is there something specific that'll break if we follow this?
On Fri, Oct 01, 2021 at 12:05:25PM -0700, Nick Desaulniers wrote: > On Thu, Sep 30, 2021 at 4:58 PM Kees Cook <keescook@chromium.org> wrote: > > > > While discussing how to format the addition of various function > > attributes, some "unwritten rules" of ordering surfaced[1]. Capture as > > close as possible to Linus's preferences for future reference. > > > > (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and > > others that would prefer all attributes live on a separate leading line.) > > > > [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/ > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > While I appreciate you getting the ball across the finish line (having > _any_ documentation to point to in future bikesheds), I can't help but > shake the feeling that the chosen policy will harm the ability of > existing automated code formatting tools from being able to automate > code formatting on the kernel. Why would documenting the expected format have an affect on tools not being able to follow that exact expected format? Are we defining a format that is somehow impossible for them to use? If anything I would think that now we have a format that the tools can actually follow, while before it was semi-random as to what to pick as the "one true way". What am I missing here? thanks, greg k-h
On Sat, Oct 2, 2021 at 8:31 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > Why would documenting the expected format have an affect on tools not > being able to follow that exact expected format? Are we defining a > format that is somehow impossible for them to use? From a quick test, clang-format-12 with our current config keeps attributes in the same line and it does not seem to reorder them, so it seems OK (the developer has to do it by hand, but that is fine) except for the `__malloc` in the second example which is in a different line (but not in the first). Is that intended? > If anything I would think that now we have a format that the tools can > actually follow, while before it was semi-random as to what to pick as > the "one true way". In the future, clang-format could have a configuration option to pass a sort order, in which case, having the sort order already defined in the kernel would definitely be helpful. In fact, we could use the fact that the kernel has one as a way to tell upstream that such a feature would be nice to have :) Cheers, Miguel
On Fri, Oct 1, 2021 at 1:59 AM Kees Cook <keescook@chromium.org> wrote: > > + static __always_inline __init __printf(4, 5) void * __must_check action(enum magic value, > + size_t size, u8 count, char *fmt, ...) > + __malloc (From my other email) Is this `__malloc` intended to be in a separate line? (the first example has everything on the same line). Cheers, Miguel
On Sat, 2021-10-02 at 08:31 +0200, Greg KH wrote: > I would think that now we have a format that the tools can > actually follow, while before it was semi-random as to what to pick as > the "one true way". There will never be 'one true (and universal) way'. Most existing code doesn't follow the suggested formatting and you can't require or expect the existing code to change. If automated scripts exist to change all the code to that new 'one true way', it wouldn't be applied. > What am I missing here? Try writing a regex for the proposal and make sure to separate out all the various __<foo> attributes into their proper locations... yuck and g'luck.
On Sat, 2021-10-02 at 12:42 +0200, Miguel Ojeda wrote: > In the future, clang-format could have a configuration option to pass > a sort order, in which case, having the sort order already defined in > the kernel would definitely be helpful. It's not so much a sort order so much as it's a positional one.
On Sat, Oct 2, 2021 at 5:22 PM Joe Perches <joe@perches.com> wrote: > > If automated scripts exist to change all the code to that new > 'one true way', it wouldn't be applied. I think nobody is saying we should reformat all code at once, just that agreeing on a given style allows us to eventually automate it (it also makes humans more likely to be consistent). > Try writing a regex for the proposal and make sure to separate out > all the various __<foo> attributes into their proper locations... It does not need to be a regex... Cheers, Miguel
On Sat, Oct 2, 2021 at 5:22 PM Joe Perches <joe@perches.com> wrote: > > It's not so much a sort order so much as it's a positional one. Sure, there are two parts, the order w.r.t. the signature parts (e.g. "before return type") and the order between the attributes themselves. Both are included in what I meant. By the way, clang-format-13 already has a way to pass it a list of the "attribute macros" (such as `__unused`), so adding more information on top seems reasonable. Cheers, Miguel
On Sat, Oct 02, 2021 at 06:27:02PM +0200, Miguel Ojeda wrote: > On Sat, Oct 2, 2021 at 5:22 PM Joe Perches <joe@perches.com> wrote: > > > > If automated scripts exist to change all the code to that new > > 'one true way', it wouldn't be applied. > > I think nobody is saying we should reformat all code at once, just > that agreeing on a given style allows us to eventually automate it (it > also makes humans more likely to be consistent). I just want to know what style to use to not have it NAKed by Linus. ;) But yeah, it might be nice to do style checking against the kernel some day. We have a lot of weird stuff in here, though! *slaps roof of kernel*
Kees Cook <keescook@chromium.org> writes: > While discussing how to format the addition of various function > attributes, some "unwritten rules" of ordering surfaced[1]. Capture as > close as possible to Linus's preferences for future reference. > > (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and > others that would prefer all attributes live on a separate leading line.) > > [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/ > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > v4: > - fix another stray "void"! This is why code needs a compiler... (thx randy) > --- > Documentation/process/coding-style.rst | 30 ++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) Did I ever mention that I *hate* coding-style patches? :) In this case I think we're as close as to consensus as things get. In the absence of a strong reason to the contrary, I'll apply this before too long. Thanks, jon
On Tue, Oct 5, 2021 at 1:57 AM Jonathan Corbet <corbet@lwn.net> wrote: > > In this case I think we're as close as to consensus as things get. In > the absence of a strong reason to the contrary, I'll apply this before > too long. No strong reason, but there was the question about the `__malloc` in a separate line in the second example which seems to contradict the declaration and it is not explained otherwise (+ clang-format does it differently). Cheers, Miguel
On Tue, Oct 05, 2021 at 10:28:40AM +0200, Miguel Ojeda wrote: > On Tue, Oct 5, 2021 at 1:57 AM Jonathan Corbet <corbet@lwn.net> wrote: > > > > In this case I think we're as close as to consensus as things get. In > > the absence of a strong reason to the contrary, I'll apply this before > > too long. > > No strong reason, but there was the question about the `__malloc` in a > separate line in the second example which seems to contradict the > declaration and it is not explained otherwise (+ clang-format does it > differently). I'll send a v5 -- the "extern" also needs to be dropped.
diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst index 42969ab37b34..45b48510f5ec 100644 --- a/Documentation/process/coding-style.rst +++ b/Documentation/process/coding-style.rst @@ -487,6 +487,36 @@ because it is a simple way to add valuable information for the reader. Do not use the ``extern`` keyword with function prototypes as this makes lines longer and isn't strictly necessary. +When writing a function declarations, please keep the `order of elements regular +<https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/>`_. +For example:: + + extern __init void * __must_check action(enum magic value, size_t size, u8 count, + char *fmt, ...) __printf(4, 5) __malloc; + +The preferred order of elements for a function prototype is: + +- storage class (here, ``extern``, and things like ``static __always_inline`` even though + ``__always_inline`` is technically an attribute, it is treated like ``inline``) +- storage class attributes (here, ``__init`` -- i.e. section declarations, but also things like ``__cold``) +- return type (here, ``void *``) +- return type attributes (here, ``__must_check``) +- function name (here, ``action``) +- function parameters (here, ``(enum magic value, size_t size, u8 count, char *fmt, ...)``, noting that parameter names should always be included) +- function parameter attributes (here, ``__printf(4, 5)``) +- function behavior attributes (here, ``__malloc``) + +Note that for a function definition (e.g. ``static inline``), the compiler does +not allow function parameter attributes after the function parameters. In these +cases, they should go after the storage class attributes (e.g. note the changed +position of ``__printf(4, 5)``):: + + static __always_inline __init __printf(4, 5) void * __must_check action(enum magic value, + size_t size, u8 count, char *fmt, ...) + __malloc + { + ... + } 7) Centralized exiting of functions -----------------------------------
While discussing how to format the addition of various function attributes, some "unwritten rules" of ordering surfaced[1]. Capture as close as possible to Linus's preferences for future reference. (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and others that would prefer all attributes live on a separate leading line.) [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/ Signed-off-by: Kees Cook <keescook@chromium.org> --- v4: - fix another stray "void"! This is why code needs a compiler... (thx randy) --- Documentation/process/coding-style.rst | 30 ++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)