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

Ariel Miculas posted 1 patch 2 years, 7 months ago
rust/helpers.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH] rust: Sort rust/helpers.c's #include directives
Posted by Ariel Miculas 2 years, 7 months ago
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
Posted by Miguel Ojeda 2 years, 7 months ago
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
Posted by Ariel Miculas 2 years, 7 months ago
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
Posted by Miguel Ojeda 2 years, 5 months ago
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
Re: [PATCH v3] rust: helpers: sort includes alphabetically in rust/helpers.c
Posted by Gary Guo 2 years, 7 months ago
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
Posted by Martin Rodriguez Reboredo 2 years, 7 months ago
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
Posted by Ariel Miculas 2 years, 7 months ago
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
Posted by Miguel Ojeda 2 years, 7 months ago
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