Suggested-by: Miguel Ojeda <[email protected]>
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(-)
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
Hi Ariel,
On Tue, Apr 25, 2023 at 10:28 PM Ariel Miculas <[email protected]> wrote:
>
> 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.
> +/*
> + * 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
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]>
---
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
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]>
---
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
My bad, I'm sending this again from my cisco account.
On Wed, Apr 26, 2023 at 11:14 AM Ariel Miculas <[email protected]> wrote:
>
> 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]>
> ---
> 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
>
On Wed, Apr 26, 2023 at 10:18 AM Ariel Miculas <[email protected]> wrote:
>
> 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
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]>
---
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
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 <[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]>
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:
http://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
<[email protected]> 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 <[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]>
On Wed, Apr 26, 2023 at 11:04 PM Ariel Miculas <[email protected]> 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:
> http://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
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]>
> ---
> 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)
On Wed, Apr 26, 2023 at 10:50 PM 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]>
Applied to `rust-next`, thanks!
Cheers,
Miguel