[PATCH] rust: Sort rust/helpers.c's #include directives

[PATCH] rust: Sort rust/helpers.c's #include directives

[Ariel Miculas]

Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Link: https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by: Ariel Miculas <amiculas@cisco.com>
---
 rust/helpers.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index 81e80261d597..59a23b1647fb 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -18,13 +18,17 @@
  * accidentally exposed.
  */
 
+/*
+ * Sorted alphabetically.
+ */
+
 #include <linux/bug.h>
 #include <linux/build_bug.h>
 #include <linux/err.h>
-#include <linux/refcount.h>
 #include <linux/mutex.h>
-#include <linux/spinlock.h>
+#include <linux/refcount.h>
 #include <linux/sched/signal.h>
+#include <linux/spinlock.h>
 #include <linux/wait.h>
 
 __noreturn void rust_helper_BUG(void)
-- 
2.40.0

Re: [PATCH] rust: Sort rust/helpers.c's #include directives

[Miguel Ojeda]

Hi Ariel,

On Tue, Apr 25, 2023 at 10:28 PM Ariel Miculas <ariel.miculas@gmail.com> wrote:
>
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Link: https://github.com/Rust-for-Linux/linux/issues/1003
> Signed-off-by: Ariel Miculas <amiculas@cisco.com>

First of all, thanks for the patch!

These tags look OK, but if you want to attribute the patches to your
Cisco address, could you please send them from that address?

Also, the commit description is missing. In the kernel, we give the
rationale for a change and describe what the patch does for every
commit.

Please take a look at `Documentation/process/submitting-patches.rst`
and try `scripts/checkpatch.pl` -- it can point some issues like this.

> +/*
> + * Sorted alphabetically.
> + */

Please join this comment with the top one, i.e. please do not open a
new /* */ block, like it is done in the other file mentioned in the
linked issue.

Cheers,
Miguel

[PATCH] rust: Sort rust/helpers.c's #include directives

[Ariel Miculas]

Sort the #include directives of rust/helpers.c alphabetically and add a
comment specifying this.

Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Link: https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by: Ariel Miculas <amiculas@cisco.com>
---
 rust/helpers.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index 81e80261d597..73c01db0c828 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -16,15 +16,17 @@
  *
  * All symbols are exported as GPL-only to guarantee no GPL-only feature is
  * accidentally exposed.
+ *
+ * Sorted alphabetically.
  */
 
 #include <linux/bug.h>
 #include <linux/build_bug.h>
 #include <linux/err.h>
-#include <linux/refcount.h>
 #include <linux/mutex.h>
-#include <linux/spinlock.h>
+#include <linux/refcount.h>
 #include <linux/sched/signal.h>
+#include <linux/spinlock.h>
 #include <linux/wait.h>
 
 __noreturn void rust_helper_BUG(void)
-- 
2.40.0

Re: [PATCH] rust: Sort rust/helpers.c's #include directives

[Ariel Miculas]

My bad, I'm sending this again from my cisco account.

On Wed, Apr 26, 2023 at 11:14 AM Ariel Miculas <ariel.miculas@gmail.com> wrote:
>
> Sort the #include directives of rust/helpers.c alphabetically and add a
> comment specifying this.
>
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Link: https://github.com/Rust-for-Linux/linux/issues/1003
> Signed-off-by: Ariel Miculas <amiculas@cisco.com>
> ---
>  rust/helpers.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 81e80261d597..73c01db0c828 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -16,15 +16,17 @@
>   *
>   * All symbols are exported as GPL-only to guarantee no GPL-only feature is
>   * accidentally exposed.
> + *
> + * Sorted alphabetically.
>   */
>
>  #include <linux/bug.h>
>  #include <linux/build_bug.h>
>  #include <linux/err.h>
> -#include <linux/refcount.h>
>  #include <linux/mutex.h>
> -#include <linux/spinlock.h>
> +#include <linux/refcount.h>
>  #include <linux/sched/signal.h>
> +#include <linux/spinlock.h>
>  #include <linux/wait.h>
>
>  __noreturn void rust_helper_BUG(void)
> --
> 2.40.0
>

[PATCH] rust: Sort rust/helpers.c's #include directives

[Ariel Miculas]

Sort the #include directives of rust/helpers.c alphabetically and add a
comment specifying this.

Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Link: https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by: Ariel Miculas <amiculas@cisco.com>
---
 rust/helpers.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index 81e80261d597..73c01db0c828 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -16,15 +16,17 @@
  *
  * All symbols are exported as GPL-only to guarantee no GPL-only feature is
  * accidentally exposed.
+ *
+ * Sorted alphabetically.
  */
 
 #include <linux/bug.h>
 #include <linux/build_bug.h>
 #include <linux/err.h>
-#include <linux/refcount.h>
 #include <linux/mutex.h>
-#include <linux/spinlock.h>
+#include <linux/refcount.h>
 #include <linux/sched/signal.h>
+#include <linux/spinlock.h>
 #include <linux/wait.h>
 
 __noreturn void rust_helper_BUG(void)
-- 
2.40.0

Re: [PATCH] rust: Sort rust/helpers.c's #include directives

[Miguel Ojeda]

On Wed, Apr 26, 2023 at 10:18 AM Ariel Miculas <amiculas@cisco.com> wrote:
>
> Sort the #include directives of rust/helpers.c alphabetically and add a
> comment specifying this.
>
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Link: https://github.com/Rust-for-Linux/linux/issues/1003
> Signed-off-by: Ariel Miculas <amiculas@cisco.com>

Looks better, and the content is fine!

A few minor notes and tips:

  - Please annotate the version of the series when you resubmit a
patch series, e.g. v2, v3, etc. `git-format-patch` can do this for you
via e.g. `-v2`.

  - Typically the title would be prefixed like "rust: helpers: sort
includes". In general, try to take a look at a subsystem's pattern for
similar commits.

  - The description should also give the rationale for the change,
i.e. the "why" and the "what". This one, for instance, could say that
this is done to improve readability and to be consistent with the
other files with a similar approach within `rust/`.

I can fix these on my side if you prefer, but it may be good practice
if you want to send a quick new version.

Thanks!

Cheers,
Miguel

[PATCH v3] rust: helpers: sort includes alphabetically in rust/helpers.c

[Ariel Miculas]

Sort the #include directives of rust/helpers.c alphabetically and add a
comment specifying this. The reason for this is to improve readability
and to be consistent with the other files with a similar approach within
'rust/'.

Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Link: https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by: Ariel Miculas <amiculas@cisco.com>
---
 rust/helpers.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/rust/helpers.c b/rust/helpers.c
index 81e80261d597..73c01db0c828 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -16,15 +16,17 @@
  *
  * All symbols are exported as GPL-only to guarantee no GPL-only feature is
  * accidentally exposed.
+ *
+ * Sorted alphabetically.
  */
 
 #include <linux/bug.h>
 #include <linux/build_bug.h>
 #include <linux/err.h>
-#include <linux/refcount.h>
 #include <linux/mutex.h>
-#include <linux/spinlock.h>
+#include <linux/refcount.h>
 #include <linux/sched/signal.h>
+#include <linux/spinlock.h>
 #include <linux/wait.h>
 
 __noreturn void rust_helper_BUG(void)
-- 
2.40.0

Re: [PATCH v3] rust: helpers: sort includes alphabetically in rust/helpers.c

[Martin Rodriguez Reboredo]

On 4/26/23 17:49, Ariel Miculas wrote:
> Sort the #include directives of rust/helpers.c alphabetically and add a
> comment specifying this. The reason for this is to improve readability
> and to be consistent with the other files with a similar approach within
> 'rust/'.
> 
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Link: https://github.com/Rust-for-Linux/linux/issues/1003
> Signed-off-by: Ariel Miculas <amiculas@cisco.com>
> ---
> [...]
Shouldn't the link use the `Link: <url> [1]` format? Otherwise LGTM.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v3] rust: helpers: sort includes alphabetically in rust/helpers.c

[Ariel Miculas]

This is the format I see in https://docs.kernel.org/process/5.Posting.html#
```
Link: https://example.com/somewhere.html  optional-other-stuff
```
And looking briefly through the git log I see the simple `Link:
www.example.com` format.
But I'm happy to change this if needed, can you point me to the right format?

On Wed, Apr 26, 2023 at 11:57 PM Martin Rodriguez Reboredo
<yakoyoku@gmail.com> wrote:
>
> On 4/26/23 17:49, Ariel Miculas wrote:
> > Sort the #include directives of rust/helpers.c alphabetically and add a
> > comment specifying this. The reason for this is to improve readability
> > and to be consistent with the other files with a similar approach within
> > 'rust/'.
> >
> > Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> > Link: https://github.com/Rust-for-Linux/linux/issues/1003
> > Signed-off-by: Ariel Miculas <amiculas@cisco.com>
> > ---
> > [...]
> Shouldn't the link use the `Link: <url> [1]` format? Otherwise LGTM.
>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v3] rust: helpers: sort includes alphabetically in rust/helpers.c

[Miguel Ojeda]

On Wed, Apr 26, 2023 at 11:04 PM Ariel Miculas <ariel.miculas@gmail.com> wrote:
>
> This is the format I see in https://docs.kernel.org/process/5.Posting.html#
> ```
> Link: https://example.com/somewhere.html  optional-other-stuff
> ```
> And looking briefly through the git log I see the simple `Link:
> www.example.com` format.

Indeed, the one you have in the commit is the right one :)

There are 5 commits in the kernel with the format mentioned by Martin,
but they were probably unintentional.

Cheers,
Miguel

Re: [PATCH v3] rust: helpers: sort includes alphabetically in rust/helpers.c

[Gary Guo]

On Wed, 26 Apr 2023 23:49:23 +0300
Ariel Miculas <amiculas@cisco.com> wrote:

> Sort the #include directives of rust/helpers.c alphabetically and add a
> comment specifying this. The reason for this is to improve readability
> and to be consistent with the other files with a similar approach within
> 'rust/'.
> 
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Link: https://github.com/Rust-for-Linux/linux/issues/1003
> Signed-off-by: Ariel Miculas <amiculas@cisco.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/helpers.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 81e80261d597..73c01db0c828 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -16,15 +16,17 @@
>   *
>   * All symbols are exported as GPL-only to guarantee no GPL-only feature is
>   * accidentally exposed.
> + *
> + * Sorted alphabetically.
>   */
>  
>  #include <linux/bug.h>
>  #include <linux/build_bug.h>
>  #include <linux/err.h>
> -#include <linux/refcount.h>
>  #include <linux/mutex.h>
> -#include <linux/spinlock.h>
> +#include <linux/refcount.h>
>  #include <linux/sched/signal.h>
> +#include <linux/spinlock.h>
>  #include <linux/wait.h>
>  
>  __noreturn void rust_helper_BUG(void)

Re: [PATCH v3] rust: helpers: sort includes alphabetically in rust/helpers.c

[Miguel Ojeda]

On Wed, Apr 26, 2023 at 10:50 PM Ariel Miculas <amiculas@cisco.com> wrote:
>
> Sort the #include directives of rust/helpers.c alphabetically and add a
> comment specifying this. The reason for this is to improve readability
> and to be consistent with the other files with a similar approach within
> 'rust/'.
>
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Link: https://github.com/Rust-for-Linux/linux/issues/1003
> Signed-off-by: Ariel Miculas <amiculas@cisco.com>

Applied to `rust-next`, thanks!

Cheers,
Miguel