Implement a check to ensure that the author, firmware, and alias fields
of the module! macro are properly formatted.
* If the array contains more than one value, enforce vertical
formatting.
* If the array contains only one value, it may be formatted on a single
line
Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
---
scripts/checkpatch.pl | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7b28ad331742..54e1893d13aa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2775,6 +2775,9 @@ sub process {
$realcnt = 0;
$linenr = 0;
$fixlinenr = -1;
+
+ my %array_parse_module;
+
foreach my $line (@lines) {
$linenr++;
$fixlinenr++;
@@ -3567,6 +3570,46 @@ sub process {
# ignore non-hunk lines and lines being removed
next if (!$hunk_line || $line =~ /^-/);
+# check if the field is about author, firmware or alias from module! macro and find malformed arrays
+ my $inline = 0;
+ my $key = "";
+ my $add_line = $line =~ /^\+/;
+
+ if ($line =~ /\b(authors|alias|firmware)\s*:\s*\[/) {
+ $inline = 1;
+ $array_parse_module{$1} = 1;
+ }
+
+ my @keys = keys %array_parse_module;
+ if (@keys) {
+ $key = $keys[0];
+ }
+
+ if ($add_line && $key) {
+ my $herevet = "$here\n" . cat_vet($rawline) . "\n";
+
+ my $counter = () = $line =~ /"/g;
+ my $more_than_one = $counter > 2;
+ if ($more_than_one) {
+ WARN("ARRAY_MODULE_MACRO",
+ "Prefer each array element on a separate line\n". $herevet);
+ } elsif ($inline && $line !~ /\]/ && $line !~ /,/ && $line =~ /"/) {
+ WARN("ARRAY_MODULE_MACRO",
+ "Prefer declare ] on the same line\n" . $herevet);
+ } elsif (!$inline && $line =~ /\]/ && $line =~ /\"/) {
+ WARN("ARRAY_MODULE_MACRO",
+ "Prefer a new line after the last value and before ]\n" . $herevet);
+ } elsif ($inline && $line =~ /,/ && $line !~ /\]/) {
+ WARN("ARRAY_MODULE_MACRO",
+ "Prefer a new line after [\n" . $herevet);
+ }
+ }
+
+ #END OF ANALYZE FIELD
+ if ($line =~ /\]/) {
+ delete $array_parse_module{$key};
+ }
+
#trailing whitespace
if ($line =~ /^\+.*\015/) {
my $herevet = "$here\n" . cat_vet($rawline) . "\n";
--
2.34.1"Guilherme Giacomo Simoes" <trintaeoitogc@gmail.com> writes:
> Implement a check to ensure that the author, firmware, and alias fields
> of the module! macro are properly formatted.
>
> * If the array contains more than one value, enforce vertical
> formatting.
> * If the array contains only one value, it may be formatted on a single
> line
>
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
Tested-by: Andreas Hindborg <a.hindborg@kernel.org>
Does what it says on the label. However, indentation is not checked and
the following is passing:
authors: [
"John Doe",
"Foo Bar"
],
Another thing: I'm wondering if we could have a rust hostprog to do
this check, rather than a perl script?
Best regards,
Andreas Hindborg
Andreas Hindborg <a.hindborg@kernel.org> wrote: > Does what it says on the label. However, indentation is not checked and > the following is passing: > > authors: [ > "John Doe", > "Foo Bar" > ], > You is right, maybe is good doing this? > Another thing: I'm wondering if we could have a rust hostprog to do > this check, rather than a perl script? hostprog like rustfmt? About this, Daniel Sedlak say: "I think we could fight with the code formatting, because when it comes to the rust macros, rustfmt is often very confused and we could end up with variations like: authors: ["author1", "author2", "author3"] or authors: [ "author1", "author2", ] " Thanks, Guilherme
On Tue, Mar 4, 2025 at 9:51 PM Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> wrote: > > hostprog like rustfmt? No, Andreas means a script written in Rust, rather than a binary that comes from the toolchain. I think it could be a good idea (it would be lovely to write the checker in Rust -- I also had a checker bot in Python from the old days of the old `rust` branch in GitHub), but `checkpatch.pl` doesn't need a built kernel, so it would be a disadvantage or at least a difference w.r.t. the usual `checkpatch.pl`, and we may not be able to call it from `checkpatch.pl`. > About this, Daniel Sedlak say: > "I think we could fight with the code formatting, because when it comes > to the rust macros, rustfmt is often very confused and we could end up > with variations like: Did you check? i.e. is it something we noticed, or something that generally happens but maybe not in this case? Is there a way to workaround or disable that (e.g. a `rustfmt` config value)? Thanks! Cheers, Miguel
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > No, Andreas means a script written in Rust, rather than a binary that > comes from the toolchain. > > I think it could be a good idea (it would be lovely to write the > checker in Rust -- I also had a checker bot in Python from the old > days of the old `rust` branch in GitHub), but `checkpatch.pl` doesn't > need a built kernel, so it would be a disadvantage or at least a > difference w.r.t. the usual `checkpatch.pl`, and we may not be able to > call it from `checkpatch.pl`. I don't know if I really understand how this would is do. > Did you check? i.e. is it something we noticed, or something that > generally happens but maybe not in this case? Is there a way to > workaround or disable that (e.g. a `rustfmt` config value)? The rustfmt have a array_width parameter [1], but with this, all arrays in rust code will have the formatting that we set. (In this case, is 1 per line). If we set the max_width, for limit the width of line, it seens for me, that arrays don't are affected. [1] https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#array_width Thanks, Guilherme
On Thu, Mar 6, 2025 at 2:59 AM Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> wrote: > > I don't know if I really understand how this would is do. Do not worry about it for this series -- it is something for the future. > The rustfmt have a array_width parameter [1], but with this, all arrays in rust > code will have the formatting that we set. (In this case, is 1 per line). Yeah, I was thinking more of something like `skip_macro_invocations` (but it is unstable) or perhaps one like `format_macro_*` that refers to arrays within macros or similar (but I don't think it exists yet). I added the former to our wishlist at https://github.com/Rust-for-Linux/linux/issues/398, anyway, since I suspect it could be useful eventually. In any case, without changing the configuration, i.e. the current status, what happens in the different cases? e.g. 1 item, 2 items, 10 items...? Thanks! Cheers, Miguel
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > In any case, without changing the configuration, i.e. the current > status, what happens in the different cases? e.g. 1 item, 2 items, 10 > items...? In current config, the formatting seems don't have a effect for arrays. i.e. this array: authors: [ "author_1", "author_2", "author_3", "author_4","author_5","author_6","author_7" ] don't is fixed by rustfmt for no format. Thanks, Guilherme
On Thu, Mar 6, 2025 at 5:08 PM Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> wrote: > > In current config, the formatting seems don't have a effect for arrays. > i.e. this array: authors: [ "author_1", "author_2", "author_3", "author_4","author_5","author_6","author_7" ] > don't is fixed by rustfmt for no format. So if I understand correctly, `rustfmt` will simply not do anything, i.e. it will always keep the array as it was -- that sounds good enough to me! Most people will write it correctly, and for those that don't, `checkpatch.pl` will likely catch the mistake -- that is all we need. Cheers, Miguel
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> So if I understand correctly, `rustfmt` will simply not do anything,
> i.e. it will always keep the array as it was -- that sounds good
> enough to me! Most people will write it correctly, and for those that
> don't, `checkpatch.pl` will likely catch the mistake -- that is all we
> need.
Ok, but, how is mentioned by Andreas Hindborg, this scenary is passing:
authors: [
"John Doe",
"Foo Bar"
],
Is good make a check for this too and senda a v7
Thanks,
Guilherme
On Thu, Mar 6, 2025 at 6:18 PM Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> wrote: > > Is good make a check for this too and senda a v7 Sure, the more the check catches, the better (as long as it is not unreasonably complex). Cheers, Miguel
On Tue, Mar 4, 2025 at 10:00 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > I think it could be a good idea (it would be lovely to write the > checker in Rust -- I also had a checker bot in Python from the old > days of the old `rust` branch in GitHub), but `checkpatch.pl` doesn't > need a built kernel, so it would be a disadvantage or at least a > difference w.r.t. the usual `checkpatch.pl`, and we may not be able to > call it from `checkpatch.pl`. By "built kernel", I don't mean an entire kernel, but rather having to call `make` in general, i.e. we could have a target to build just the script, but still, it is a difference. Cheers, Miguel
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes: > On Tue, Mar 4, 2025 at 10:00 PM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: >> >> I think it could be a good idea (it would be lovely to write the >> checker in Rust -- I also had a checker bot in Python from the old >> days of the old `rust` branch in GitHub), but `checkpatch.pl` doesn't >> need a built kernel, so it would be a disadvantage or at least a >> difference w.r.t. the usual `checkpatch.pl`, and we may not be able to >> call it from `checkpatch.pl`. > > By "built kernel", I don't mean an entire kernel, but rather having to > call `make` in general, i.e. we could have a target to build just the > script, but still, it is a difference. Right, it needs a bit more tool support than running checkpatch.pl needs. Perhaps we could move it from checkpatch.pl to the rustfmt make target? Best regards, Andreas Hindborg
On Wed, Mar 5, 2025 at 8:53 AM Andreas Hindborg <a.hindborg@kernel.org> wrote: > > Right, it needs a bit more tool support than running checkpatch.pl > needs. Perhaps we could move it from checkpatch.pl to the rustfmt make > target? That could perhaps be an option for this case, though not sure if it applies to all cases, i.e. `checkpatch.pl` also checks things that only make sense to check in a patch and also things that are not related to formatting. Perhaps we want an entirely separate thing in `tools/` eventually, or even out of the kernel tree, so that it can be easily run as a bot etc. like in the past. In any case, landing checks here is fine (as long as Joe et al. agree), they can be moved or removed later if needed. Cheers, Miguel
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> In any case, landing checks here is fine (as long as Joe et al.
> agree), they can be moved or removed later if needed.
The only thing bad in this check, is when the array is very very big, like:
authors: [
"author_1"
"author_2"
"author_3"
"author_4"
"author_5"
"author_6"
"author_8"
"author_9"
"author_10"
"author_11"
"author_12"
"author_13"
"author_14"
"author_15"
"author_16"
"author_17"
],
if I set a new author_18, the diff will be:
"author_15"
"author_16"
"author_17"
+ "author_18"
],
And, in this case, if I make a wrong change, like:
"author_15"
"author_16"
"author_17"
+ "author_18" , "author_19"
],
the checkpatch don't will throw a warning message, because, he don't can
perceive that he is within of array of author, firmware or alias of module!.
I couldn't do a better check for this..
thoughs?
Thanks,
Guilherme
On Thu, Mar 6, 2025 at 5:32 PM Guilherme Giacomo Simoes <trintaeoitogc@gmail.com> wrote: > > the checkpatch don't will throw a warning message, because, he don't can > perceive that he is within of array of author, firmware or alias of module!. I wouldn't worry about that case -- it is unlikely we even have the case, and even if we do, just manually checking it is fine. :) Cheers, Miguel
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > Perhaps we want an entirely separate thing in `tools/` eventually, or > even out of the kernel tree, so that it can be easily run as a bot > etc. like in the past. A out of kernel tree tool, is a good idea for me. But a separated tool in `tools/`, don't seems a good ideia for me, because, we have things a lot for do in addition on developing our code. like: - make LLVM=1 rusttest - ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 --kconfig_add CONFIG_RUST=y - ./scripts/checkpatch.pl ... and anothers things more.... I don't think that this things is uneccessary, but, if we might reduce the work of developer, and set he for focus on your code, seem better for me. This is only my opinion that can be absolutely wrong... Thanks, Guilherme
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes: > On Wed, Mar 5, 2025 at 8:53 AM Andreas Hindborg <a.hindborg@kernel.org> wrote: >> >> Right, it needs a bit more tool support than running checkpatch.pl >> needs. Perhaps we could move it from checkpatch.pl to the rustfmt make >> target? > > That could perhaps be an option for this case, though not sure if it > applies to all cases, i.e. `checkpatch.pl` also checks things that > only make sense to check in a patch and also things that are not > related to formatting. > > Perhaps we want an entirely separate thing in `tools/` eventually, or > even out of the kernel tree, so that it can be easily run as a bot > etc. like in the past. > > In any case, landing checks here is fine (as long as Joe et al. > agree), they can be moved or removed later if needed. Absolutely. Just brainstorming at this point. Best regards, Andreas Hindborg
© 2016 - 2025 Red Hat, Inc.