linux-kernel - Re: [PATCH] rust: add `assert

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72m04yAwxtMbctCpiDpYzoMHpiDagEJsvHHJCZNJXyJ5KQ@mail.gmail.com>
Date: Sat, 7 Jun 2025 19:29:58 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Christian Schrefl <chrisi.schrefl@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH] rust: add `assert_sync` function

On Sat, Jun 7, 2025 at 3:02 PM Christian Schrefl
<chrisi.schrefl@...il.com> wrote:
>
> +/// Asserts that the given type is [`Sync`]. This check is done at compile time and does nothing
> +/// at runtime.

I would split the second sentence into its own paragraph, so that the
"short description" isn't long.

> +/// Note that this is only intended to avoid regressions and for sanity checks.

Hmm... I am not sure about this sentence. A macro may want to call
this to ensure something that is required for safety, for instance. Is
that the "sanity check" part? In any case, it sounds like the sentence
could be read as "this is not reliable for "other" things apart from
just sanity checks", which may be confusing.

Could we perhaps clarify?

> +/// # Examples
> +/// ```

Please add a newline between these.

> +///
> +///

These newlines should be removed, otherwise they will be rendered.

> +/// // Do the assertion in a const block to make sure it won't be executed at runtime.
> +/// const _:() = {
> +///     assert_sync::<i32>(); // Succeeds because `i32` is Sync

`Sync` and please use a period at the end. Also, I would suggest
following our usual style and putting it at the top, i.e.

    // Succeeds because `i32` is `Sync`.
    assert_sync::<i32>();

> +///
> +/// ```

This one can be removed too.

Cheers,
Miguel