From: Brendan Shephard <bshephar@bne-home.net>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc: dakr@kernel.org, acourbot@nvidia.com, airlied@gmail.com,
aliceryhl@google.com, daniel.almeida@collabora.com,
rust-for-linux@vger.kernel.org
Subject: Re: [PATCH v2] rust: Return Option from page_align and ensure no usize overflow
Date: Fri, 28 Nov 2025 13:35:21 +1000 [thread overview]
Message-ID: <aSkYeanuMeltfGGm@fedora> (raw)
In-Reply-To: <CANiq72m0Z5ax3_ABSCo6Ogpeuv3u4aGsK2Fixx_q5mAMaNcy3w@mail.gmail.com>
On Thu, Nov 27, 2025 at 05:24:54PM +0100, Miguel Ojeda wrote:
> On Thu, Nov 27, 2025 at 3:41 PM Brendan Shephard <bshephar@bne-home.net> wrote:
> >
> > Changes in v2:
> > - Reworded commit message to follow the imperative form.
> > - Expanded the documentation to explain the `Some` and `None` return cases.
> > - Added a period at the end of the documentation comment.
>
> This shouldn't be in the commit message. It should instead be placed
> below the first `---` line (the one below the Signed-off-by).
>
> Also, I would recommend not replying to the previous version but
> starting a new thread instead (it is good to provide a lore.kernel.org
> to the previous version in the change log).
>
> In addition, if possible, it is nice to use `--base` to tell Git to
> provide the base commit.
>
> > +/// Return Some [`usize`] that is page aligned. Or None in cases where the next multiple of
> > +/// [`PAGE_SIZE`] would overflow a [`usize`].
>
> Missing intra-doc link (this was feedback in v1). In addition, this
> reads a bit strangely, and it looks like the first line of the
> description wasn't removed? That would make the "short description"
> (the first paragraph) quite long. Please generate the docs to see if
> it looks good when generated.
>
> More importantly, please add some examples (doctests). They are tested
> as KUnit tests automatically. For new APIs, we always try to do add
> examples, and in cases where there are edge cases like here, it
> clarifies quite a bit, i.e. please provide a representative example of
> the `Some` case and the `None` case.
>
> Thanks!
>
> Cheers,
> Miguel
This is all really good feedback Miguel, thanks for the pointers. I
really like the idea of adding examples. Let me work on that, and then
I'll send a V3 as a new thread with references to this one via the
lore.kernel.org link.
prev parent reply other threads:[~2025-11-28 3:35 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-27 13:07 [PATCH] rust: Return Option from page_align and ensure no usize overflow Brendan Shephard
2025-11-27 13:42 ` Alexandre Courbot
2025-11-27 14:18 ` Brendan Shephard
2025-11-28 0:28 ` Alexandre Courbot
2025-11-28 2:12 ` Miguel Ojeda
2025-11-28 5:29 ` Brendan Shephard
2025-11-28 6:51 ` Alexandre Courbot
2025-11-28 9:09 ` Alice Ryhl
2025-11-28 13:34 ` Alexandre Courbot
2025-11-27 13:44 ` Daniel Almeida
2025-11-27 14:21 ` Brendan Shephard
2025-11-27 14:40 ` [PATCH v2] " Brendan Shephard
2025-11-27 16:24 ` Miguel Ojeda
2025-11-28 3:35 ` Brendan Shephard [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aSkYeanuMeltfGGm@fedora \
--to=bshephar@bne-home.net \
--cc=acourbot@nvidia.com \
--cc=airlied@gmail.com \
--cc=aliceryhl@google.com \
--cc=dakr@kernel.org \
--cc=daniel.almeida@collabora.com \
--cc=miguel.ojeda.sandonis@gmail.com \
--cc=rust-for-linux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.