[11/13] rust: kernel: add doclinks with html tags

Message ID 20240116231118.168882-1-kernel@valentinobst.de
State New
Headers
Series rust: kernel: documentation improvements |

Commit Message

Valentin Obst Jan. 16, 2024, 11:11 p.m. UTC
  Add doclinks to existing documentation. Use html 'code' tags to add
links to items that cannot be linked with the normal syntax.

The use of html tags is a tradeoff between the readability of the
documentation's source code and the ergonomics of the generated content.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/str.rs       |  7 ++++---
 rust/kernel/sync/arc.rs  | 24 +++++++++++++-----------
 rust/kernel/workqueue.rs | 10 +++++-----
 3 files changed, 22 insertions(+), 19 deletions(-)
  

Comments

Martin Rodriguez Reboredo Jan. 17, 2024, 1:47 a.m. UTC | #1
On 1/16/24 20:11, Valentin Obst wrote:
> Add doclinks to existing documentation. Use html 'code' tags to add
> links to items that cannot be linked with the normal syntax.
> 
> The use of html tags is a tradeoff between the readability of the
> documentation's source code and the ergonomics of the generated content.
> 
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
> [...]
> @@ -14,7 +14,8 @@
>   
>   /// Byte string without UTF-8 validity guarantee.
>   ///
> -/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
> +/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
> +/// semantical meaning.

Isn't there a way to escape square brackets with backslashes with
mbBook? Like `\[qux\]` or something? I ask this because this affects the
readability of the doc comment so if that could be omitted it'll be
really good.

>   pub type BStr = [u8];
>   
>   /// Creates a new [`BStr`] from a string literal.
> [...]
  
Valentin Obst Jan. 17, 2024, 9:10 a.m. UTC | #2
> > [...]
> > @@ -14,7 +14,8 @@
> >     /// Byte string without UTF-8 validity guarantee.
> >   ///
> > -/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
> > +/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
> > +/// semantical meaning.

> Isn't there a way to escape square brackets with backslashes with
> mbBook? Like `\[qux\]` or something? I ask this because this affects the
> readability of the doc comment so if that could be omitted it'll be
> really good.

Here are the things that I tried that did not produce a link:
[`[u8]`], `[[u8]]`, `[\[u8\]]`, `\[u8\]`, [`\[u8\]`], `[[u8](u8)]`,
`[[u8][u8]]`,

This results in a link, but it includes the square brackets:
[`[u8]`][u8], [`[u8]`](u8)

This results in a link that only includes the `u8`, but it is not
formatted as code:
[[u8]]

The only other examples of linked slices that I found are in the
standard library [1].

My assuption is that crate documentation is much more often consumed in
its rendered form, which is why I think the reduced readability is ok.
However, if that is not the case this change might be a bad idea.

[1]: https://doc.rust-lang.org/src/alloc/ffi/c_str.rs.html#58
  
Martin Rodriguez Reboredo Jan. 17, 2024, 9:41 p.m. UTC | #3
On 1/17/24 06:10, Valentin Obst wrote:
>>> [...]
>>> @@ -14,7 +14,8 @@
>>>      /// Byte string without UTF-8 validity guarantee.
>>>    ///
>>> -/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
>>> +/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
>>> +/// semantical meaning.
> 
>> Isn't there a way to escape square brackets with backslashes with
>> mbBook? Like `\[qux\]` or something? I ask this because this affects the
>> readability of the doc comment so if that could be omitted it'll be
>> really good.
> 
> Here are the things that I tried that did not produce a link:
> [`[u8]`], `[[u8]]`, `[\[u8\]]`, `\[u8\]`, [`\[u8\]`], `[[u8](u8)]`,
> `[[u8][u8]]`,
> 
> This results in a link, but it includes the square brackets:
> [`[u8]`][u8], [`[u8]`](u8)
> 
> This results in a link that only includes the `u8`, but it is not
> formatted as code:
> [[u8]]
> 
> The only other examples of linked slices that I found are in the
> standard library [1].
> 
> My assuption is that crate documentation is much more often consumed in
> its rendered form, which is why I think the reduced readability is ok.
> However, if that is not the case this change might be a bad idea.
> 
> [1]: https://doc.rust-lang.org/src/alloc/ffi/c_str.rs.html#58

I have an idea, let's just omit links to sliced types if they already
have their underlying type linked nearby. As for `[u8]` I think that we
can omit it too since readers of the documentation should be
familiarized with slices.
  
Trevor Gross Jan. 18, 2024, 2:28 a.m. UTC | #4
On Tue, Jan 16, 2024 at 6:11 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Add doclinks to existing documentation. Use html 'code' tags to add
> links to items that cannot be linked with the normal syntax.
>
> The use of html tags is a tradeoff between the readability of the
> documentation's source code and the ergonomics of the generated content.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/str.rs       |  7 ++++---
>  rust/kernel/sync/arc.rs  | 24 +++++++++++++-----------
>  rust/kernel/workqueue.rs | 10 +++++-----
>  3 files changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index fec5c4314758..f95fd2ba19fb 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -14,7 +14,8 @@
>
>  /// Byte string without UTF-8 validity guarantee.
>  ///
> -/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
> +/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
> +/// semantical meaning.
>
> [...]
>
>  /// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
> -/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
> -/// to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates this double
> -/// indirection while still allowing one to increment the refcount and getting an [`Arc<T>`] when/if
> -/// needed.
> +/// over <code>&[`Arc<T>`]</code> because the latter results in a double-indirection: a pointer
> +/// (shared reference) to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates
> +/// this double indirection while still allowing one to increment the refcount and getting an
> +/// [`Arc<T>`] when/if needed.

Std sometimes does something like this, which links to the slice primitive.

    [`&[u8]`](prim@slice)

What exactly is going on with Arc, is it not getting linked correctly
when it has generics? I don't quite follow what <code> does.

I agree with Martin, we don't need to try too hard to link these types
- slices and numeric types are background knowledge, and it is easy
enough to search for the other types (Arc, Test) if the links don't
work for whatever reason.

If rustdoc just isn't making good choices in certain places or isn't
flexible enough, could you write issues in the Rust repo? Better to
get inconveniences fixed upstream if possible.

- Trevor
  
Valentin Obst Jan. 18, 2024, 9:14 a.m. UTC | #5
> Std sometimes does something like this, which links to the slice primitive.
>
>     [`&[u8]`](prim@slice)

This would indeed link `&[u8]` to the slice type. But I agree, both,
linking to slice and to `u8` is not necessary as it is common knowledge.
However, if it is a slice over a more complicated/custom type it might
be worth linking to it, and in that case the 'code' tag syntax would be
the only option we have at the moment.

> What exactly is going on with Arc, is it not getting linked correctly
> when it has generics? I don't quite follow what <code> does.

In this case it is about the `&`:

<code>&[`Arc<T>`]</code>

Here, `&Arc<T>` is formatted as code, but only the `Arc<T>` is a
clickable link. While

[`&Arc<T>`]

results in:

```
/// over [&`Arc<T>`] because the latter results in a double-indirection: a pointer
    |           ^^^^^^^^ no item named `&Arc` in scope
```

using:

&[`Arc<T>`]

would result in a link, but `&` is not formatted as code. Finally,

[`&Arc<T>`](Arc)

would work but `&` is part of the clickable link now. Thus,
using the html tag here is the only way I found to get the
'cleanest' form in the rendered document.

> If rustdoc just isn't making good choices in certain places or isn't
> flexible enough, could you write issues in the Rust repo? Better to
> get inconveniences fixed upstream if possible.

I like the idea of finding a proper solution to that in rustdoc
instead of cluttering the source code with html tags. If nobody
strongly objects I'd drop the whole patch in v2 and open an issue
in the rust repo.

	- Valentin
  

Patch

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index fec5c4314758..f95fd2ba19fb 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -14,7 +14,8 @@ 
 
 /// Byte string without UTF-8 validity guarantee.
 ///
-/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
+/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
+/// semantical meaning.
 pub type BStr = [u8];
 
 /// Creates a new [`BStr`] from a string literal.
@@ -106,7 +107,7 @@  pub unsafe fn from_char_ptr<'a>(ptr: *const core::ffi::c_char) -> &'a Self {
         unsafe { Self::from_bytes_with_nul_unchecked(bytes) }
     }
 
-    /// Creates a [`CStr`] from a `[u8]`.
+    /// Creates a [`CStr`] from a <code>[[u8]]</code>.
     ///
     /// The provided slice must be `NUL`-terminated, does not contain any
     /// interior `NUL` bytes.
@@ -130,7 +131,7 @@  pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError
         Ok(unsafe { Self::from_bytes_with_nul_unchecked(bytes) })
     }
 
-    /// Creates a [`CStr`] from a `[u8]` without performing any additional
+    /// Creates a [`CStr`] from a <code>[[u8]]</code> without performing any additional
     /// checks.
     ///
     /// # Safety
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 936bc549a082..5fcd4b0fd84b 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -368,10 +368,10 @@  fn from(item: Pin<UniqueArc<T>>) -> Self {
 /// to use just `&T`, which we can trivially get from an [`Arc<T>`] instance.
 ///
 /// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
-/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
-/// to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates this double
-/// indirection while still allowing one to increment the refcount and getting an [`Arc<T>`] when/if
-/// needed.
+/// over <code>&[`Arc<T>`]</code> because the latter results in a double-indirection: a pointer
+/// (shared reference) to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates
+/// this double indirection while still allowing one to increment the refcount and getting an
+/// [`Arc<T>`] when/if needed.
 ///
 /// # Invariants
 ///
@@ -489,8 +489,8 @@  fn deref(&self) -> &Self::Target {
 /// # Examples
 ///
 /// In the following example, we make changes to the inner object before turning it into an
-/// `Arc<Test>` object (after which point, it cannot be mutated directly). Note that `x.into()`
-/// cannot fail.
+/// <code>[Arc]\<Test\></code> object (after which point, it cannot be mutated directly).
+/// Note that `x.into()` cannot fail.
 ///
 /// ```
 /// use kernel::sync::{Arc, UniqueArc};
@@ -512,8 +512,9 @@  fn deref(&self) -> &Self::Target {
 ///
 /// In the following example we first allocate memory for a refcounted `Example` but we don't
 /// initialise it on allocation. We do initialise it later with a call to [`UniqueArc::write`],
-/// followed by a conversion to `Arc<Example>`. This is particularly useful when allocation happens
-/// in one context (e.g., sleepable) and initialisation in another (e.g., atomic):
+/// followed by a conversion to <code>[Arc]\<Example\></code>. This is particularly useful when
+/// allocation happens in one context (e.g., sleepable) and initialisation in another
+/// (e.g., atomic):
 ///
 /// ```
 /// use kernel::sync::{Arc, UniqueArc};
@@ -532,8 +533,8 @@  fn deref(&self) -> &Self::Target {
 /// ```
 ///
 /// In the last example below, the caller gets a pinned instance of `Example` while converting to
-/// `Arc<Example>`; this is useful in scenarios where one needs a pinned reference during
-/// initialisation, for example, when initialising fields that are wrapped in locks.
+/// <code>[Arc]\<Example\></code>; this is useful in scenarios where one needs a pinned reference
+/// during initialisation, for example, when initialising fields that are wrapped in locks.
 ///
 /// ```
 /// use kernel::sync::{Arc, UniqueArc};
@@ -582,7 +583,8 @@  pub fn try_new_uninit() -> Result<UniqueArc<MaybeUninit<T>>, AllocError> {
 }
 
 impl<T> UniqueArc<MaybeUninit<T>> {
-    /// Converts a `UniqueArc<MaybeUninit<T>>` into a `UniqueArc<T>` by writing a value into it.
+    /// Converts a <code>UniqueArc<[`MaybeUninit<T>`]></code> into a `UniqueArc<T>`
+    /// by writing a value into it.
     pub fn write(mut self, value: T) -> UniqueArc<T> {
         self.deref_mut().write(value);
         // SAFETY: We just wrote the value to be initialized.
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index ed3af3491b47..aedf47f258bd 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -294,9 +294,9 @@  unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
 
 /// Defines the method that should be called directly when a work item is executed.
 ///
-/// This trait is implemented by `Pin<Box<T>>` and [`Arc<T>`], and is mainly intended to be
-/// implemented for smart pointer types. For your own structs, you would implement [`WorkItem`]
-/// instead. The [`run`] method on this trait will usually just perform the appropriate
+/// This trait is implemented by <code>[Pin]<[`Box<T>`]></code> and [`Arc<T>`], and is mainly
+/// intended to be implemented for smart pointer types. For your own structs, you would implement
+/// [`WorkItem`] instead. The [`run`] method on this trait will usually just perform the appropriate
 /// `container_of` translation and then call into the [`run`][WorkItem::run] method from the
 /// [`WorkItem`] trait.
 ///
@@ -325,8 +325,8 @@  pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
 ///
 /// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
 pub trait WorkItem<const ID: u64 = 0> {
-    /// The pointer type that this struct is wrapped in. This will typically be `Arc<Self>` or
-    /// `Pin<Box<Self>>`.
+    /// The pointer type that this struct is wrapped in. This will typically be
+    /// <code>[Arc]\<Self\></code> or <code>[Pin]<[Box]\<Self\>></code>.
     type Pointer: WorkItemPointer<ID>;
 
     /// The method that should be called when this work item is executed.