Date: Sat, 30 Mar 2024 21:23:39 +0100
From: Markus Elfring <Markus.Elfring@....de>
To: Dan Williams <dan.j.williams@...el.com>,
 Peter Zijlstra <peterz@...radead.org>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
 kernel-janitors@...r.kernel.org
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Ira Weiny <ira.weiny@...el.com>,
 Jesse Brandeburg <jesse.brandeburg@...el.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Julia Lawall <Julia.Lawall@...ia.fr>, Kevin Tian <kevin.tian@...el.com>,
 Lukas Wunner <lukas.wunner@...el.com>, Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH v3] cleanup: Add usage and style documentation

> Changes since v2:
> * remove the unnecessary newlines around code examples further reducing
>   the visual interruption of RST metadata (Jon)
> * Fix a missing FILO=>LIFO conversion
> * Fix a bug in the example

I find such improvements nice.


> * Collect Jonathan's reviewed-by

How good does this action fit to the event that you pointed issues out also yourself?


> Review has been quiet on this thread for a few days so I think is the
> final rev.

I got an other impression.


> Let me know if anything else to fix up.

I would appreciate if further patch review comments can get more attention.


…
> +++ b/include/linux/cleanup.h
> @@ -4,6 +4,142 @@
>
>  #include <linux/compiler.h>
>
> +/**
> + * DOC: scope-based cleanup helpers
> + *
> + * The "goto error" pattern is notorious for introducing …

Will any other label become more helpful for this description approach?


> + * this tedium …

Would an other wording be more appropriate here?




> + *                                                       … If a function
> + * wants to invoke pci_dev_put() on error, but return @dev (i.e. without
> + * freeing it) on success, it can do:
> + *
> + * ::
> + *
> + *	return no_free_ptr(dev);
> + *
> + * ...or:
> + *
> + * ::
> + *
> + *	return_ptr(dev);
…

Would this macro call be preferred as a succinct specification
(so that only the shorter one should be mentioned here)?
https://elixir.bootlin.com/linux/v6.8.2/source/include/linux/cleanup.h#L78


> + * Observe the lock is held for the remainder of the "if ()" block not
> + * the remainder of "func()".

I suggest to add a word in this sentence.

* Observe the lock is held for the remainder of the "if ()" block
* (and not the remainder of "func()").


> + * That bug is fixed by changing init() to call guard() and define +
> + * initialize @obj in this order::
> + *
> + *	guard(mutex)(&lock);
> + *	struct object *obj __free(remove_free) = alloc_add();

It is helpful to point such a design possibility and preference out.

But I imagine that the abstraction level should be raised another bit.
It seems that the mentioned variable definition should be achieved by
calling the macro “CLASS” instead for “an instance of the named class”.
Thus the macro “DEFINE_CLASS” should also be called before accordingly.
https://elixir.bootlin.com/linux/v6.8.2/source/include/linux/cleanup.h#L82


> + * the top of the function poses this potential interdependency problem

I suggest to add a comma at the end of this line.


> + * the recommendation is to always define and assign variables in one
> + * statement and not group variable definitions at the top of the
> + * function when __free() is used.

I became curious how code layout guidance will evolve further also
according to such an advice.


> + * Lastly, given that the benefit of cleanup helpers is removal of
> + * "goto", and that the "goto" statement can jump between scopes, the
> + * expectation is that usage of "goto" and cleanup helpers is never
> + * mixed in the same function. I.e. for a given routine, convert all
> + * resources that need a "goto" cleanup to scope-based cleanup, or
> + * convert none of them.

Can the word wrapping become nicer another bit?

* Lastly, given that the benefit of cleanup helpers is removal of "goto",
* and that the "goto" statement can jump between scopes,
* the expectation is that usage of "goto" and cleanup helpers is never
* mixed in the same function. I.e. for a given routine, convert all
* resources that need a "goto" cleanup to scope-based cleanup,
* or convert none of them.


Regards,
Markus

Powered by blists - more mailing lists