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

PATCH In review Ariel Miculas 5 participants 12 messages Apr 25, 2023 – Jul 19, 2023 Rust

lore

Share & tools

lore

Ariel Miculas

author

2023-04-25 20:28 UTC #0 lore raw

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

Link https://github.com/Rust-for-Linux/linux/issues/1003

Signed-off-by Ariel Miculas <[email protected]>

---
 rust/helpers.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

rust/helpers.c +6 -2

index 81e80261d597..59a23b1647fb 100644
@@ -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)

Signature · 1 line

2.40.0

Miguel Ojeda

2023-04-25 20:38 UTC #1 lore raw

Hi Ariel,

Quoted 4 lines · Ariel Miculas

Suggested-by Miguel Ojeda <[email protected]>
Link https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by Ariel Miculas <[email protected]>

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.

Quoted 3 lines

+/*
+ * 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

Ariel Miculas

author

2023-04-26 08:15 UTC #2 lore raw

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

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

Link https://github.com/Rust-for-Linux/linux/issues/1003

Signed-off-by Ariel Miculas <[email protected]>

---
 rust/helpers.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

rust/helpers.c +4 -2

index 81e80261d597..73c01db0c828 100644
@@ -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)

Signature · 1 line

2.40.0

Ariel Miculas

2023-04-26 08:18 UTC #4 lore raw

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

In reply to Miguel Ojeda

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

Link https://github.com/Rust-for-Linux/linux/issues/1003

Signed-off-by Ariel Miculas <[email protected]>

---
 rust/helpers.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

rust/helpers.c +4 -2

index 81e80261d597..73c01db0c828 100644
@@ -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)

Signature · 1 line

2.40.0

Miguel Ojeda

2023-04-26 17:56 UTC #5 lore raw

Quoted 7 lines · Ariel Miculas

Sort the #include directives of rust/helpers.c alphabetically and add a comment specifying this.
Suggested-by Miguel Ojeda <[email protected]>
Link https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by Ariel Miculas <[email protected]>

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

Ariel Miculas

2023-04-26 20:49 UTC #6 lore raw

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

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/'.

Link https://github.com/Rust-for-Linux/linux/issues/1003

Signed-off-by Ariel Miculas <[email protected]>

---
 rust/helpers.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

rust/helpers.c +4 -2

index 81e80261d597..73c01db0c828 100644
@@ -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)

Signature · 1 line

2.40.0

Martin Rodriguez Reboredo

Reviewed-by on v3 1/1

2023-04-26 20:55 UTC #7 lore raw

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

Quoted 10 lines · 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 <[email protected]>
Link https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by Ariel Miculas <[email protected]>
---
[...]

Shouldn't the link use the Link: <url> [1] format? Otherwise LGTM.

Reviewed-by Martin Rodriguez Reboredo <[email protected]>

Ariel Miculas

author

2023-04-26 21:04 UTC #8 lore raw

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

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?

Quoted 15 lines · Martin Rodriguez Reboredo

Quoted 10 lines · 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 <[email protected]>
Link https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by Ariel Miculas <[email protected]>
---
[...]
Shouldn't the link use the Link: <url> [1] format? Otherwise LGTM.
Reviewed-by Martin Rodriguez Reboredo <[email protected]>

Miguel Ojeda

2023-04-26 21:28 UTC #9 lore raw

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

Quoted 7 lines · 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.

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

Gary Guo

reviewer Reviewed-by on v3 1/1

2023-04-27 12:37 UTC #10 lore raw

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

In reply to Ariel Miculas

Quoted 8 lines · On Wed, 26 Apr 2023 23:49:23 +0300 Ariel Miculas <[email protected]> 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 <[email protected]>
Link https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by Ariel Miculas <[email protected]>

Reviewed-by Gary Guo <[email protected]>

Quoted 28 lines

---
 rust/helpers.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

rust/helpers.c +4 -2

index 81e80261d597..73c01db0c828 100644
@@ -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)

Miguel Ojeda

Applied

2023-07-19 20:01 UTC #11 lore raw

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

In reply to Ariel Miculas

Quoted 9 lines · 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 <[email protected]>
Link https://github.com/Rust-for-Linux/linux/issues/1003
Signed-off-by Ariel Miculas <[email protected]>

Applied to rust-next, thanks!

Cheers,
Miguel