Rust-for-Linux / linux

Adding support for the Rust language to the Linux kernel.
https://rust-for-linux.com
Other
3.83k stars 399 forks source link

[RFC] Documentation and comment fixes in kernel crate #1055

Closed vobst closed 4 months ago

vobst commented 5 months ago

Hi all!

To get familiar with the Rust kernel infrastructure I read through the existing in-tree code. Along the way I fixed some trivial things in comments and documentation, as well as making some small improvements by adding doclinks. This PR only looks at the kernel crate.

Since this would be my first upstream contribution I'd appreciate some upfront review before I submit to the mailing list. In particular:

Thanks!

charmitro commented 5 months ago
* are the commit messages OK?

Two notes in the case you send this upstream via mailing list.

  1. The commits in github are from an unknown profile. You can fix it by modifying your git config.
  2. You should add Signed-off-by: ... in the commits.

In general, if you fix your git config, signing the commits can be done automatically via git commit -s.

bjorn3 commented 5 months ago

The commits in github are from an unknown profile.

If you actually look at the commits you will see that the author name and email are set correctly. They probably didn't add the email they used to their github account, so github doesn't show the associated github account as author.

vobst commented 5 months ago

rusttest only runs the few #[test]s we have. For doctests, you want to use KUnit (make sure you have CONFIG_RUST_KERNEL_DOCTESTS=y). Please see https://docs.kernel.org/next/rust/general-information.html#testing.

Ah, ok, I was already suspecting that this wasn't the right thing. Will do that.

I think you did commits by area in some cases, which is fine, but for trivial-enough things that apply to more places, you can do a single commit for everything. For instance rust: init: fix multiple typos in documentation could be rust: kernel: fix multiple typos in documentation and, given its "generic" title, it could take typos from some of the other commits perhaps (e.g. the one from rust: workqueue: fix trivial typo in docs of __enqueue).

Sure, I'll squash those trivial typofixes.

Also, in rust: workqueue: add doclinks, you also remove others. So the title should probably be different (e.g. "improve"?). Or you can keep the title and do the removals in another commit (and, again, ideally applied to more than just workqueue).

Afaik this commit should only add doc links. The removals are cases where it is not necessary to give the explicit link target as rustdoc can figure it out itself. They could go into their own commit though.

Also, you can use Markdown in commit messages if you wish (in the Rust subsystem we typically do, but different people in the kernel use different styles).

Nice, I prefer Markdown as well.

vobst commented 5 months ago

@charmitro @bjorn3 Yes, I did not add that email address to my GH account.

vobst commented 5 months ago

I'll include the changelog between this RFC and the v1 I submit to the mailing list below to aid the reviewers that already gave feedback here:

This time I also properly ran the doctests.

Edit: the v1 tree is here