Re: [PATCH 5/8] rust: debugfs: Support format hooks

Miguel Ojeda
2025-04-30T03:17:47Z

On Wed, Apr 30, 2025 at 1:16 AM Matthew Maurer <mmaurer@google.com> wrote:
>
> + /// Create a file in a DebugFS directory with the provided name, and contents from invoking `f`
> + /// on the provided reference. `f` must be a function item or a non-capturing closure, or this
> + /// will fail to compile.

The first paragraph of docs are the "title" ("short description"),
which renders differently. Typically it should be kept short, e.g. a
sentence. So I would probably move the second sentence to a second
paragraph.

In any case, is it true that it will fail to compile? Please see below.

> + /// # Example

We use plurals for these sections.

> + ) -> Result<()> {

Can be `Result`.

> + // We forget the reference because its reference count is implicitly "owned" by the root
> + // builder, which we know will use `debugfs_remove` to clean this up. If we release the
> + // file here, it will be immediately deleted.
> + // SAFETY:
> + // Because `Builder`'s invariant says that our lifetime is how long the directory will

In our usual style, this would be e.g.:

// file here, it will be immediately deleted.
//
// SAFETY: Because `Builder`'s ...

> +// INVARIANT: F is inhabited
> +#[repr(transparent)]
> +struct FormatAdapter<T, F> {

For invariants, we put them in the docs, as a section, e.g.

/// # Invariants
///
/// `F` is inhabited.

> + // SAFETY: FormatAdapater is a repr(transparent) wrapper around T, so
> + // casting a reference is legal

Please use Markdown in comments too.

> +/// # Safety
> +/// The caller asserts that F is inhabited
> +unsafe fn materialize_zst_fmt<F>() -> &'static F {

/// # Safety
///
/// `F` must be inhabited.

(I simplified the wording)

> + // We don't have generic_const_exprs, and const items inside the function get promoted out and
> + // lose type variables, so we need to do the old-style assert to check for ZSTness
> + [(); 1][core::mem::size_of::<F>()];

Shouldn't this be:

const { assert!(core::mem::size_of::<F>() == 0) };

to prevent a runtime panic?

Or `build_assert!` otherwise, but the line above catches the issue if
I try to pass a capturing closure from the sample module.

Thanks!

Cheers,
Miguel