Re: [PATCH 01/10] checkstack: fix perlcritic warnings

Re: [PATCH 01/10] checkstack: fix perlcritic warnings

[Cong Wang]

Stephen Hemminger wrote:
 >Turn on strict checking.
 >Get rid of prototypes (prototypes are broken in Perl)

NAK, please check perldoc of sort().
The prototype there is needed by sort().

 >Fix syntax of declaration.

Yeah, this is fine.

BTW, your $subject needs to be fixed too.

[PATCH] scripts: improve checkstack

[Stephen Hemminger]

Turn on strict checking, and get rid of annoying use of prototype.
Fix syntax error in declaration

Use efficient sort algorithm by using schwartzian transform.
  http://en.wikipedia.org/wiki/Schwartzian_transform

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/scripts/checkstack.pl	2010-02-19 21:57:57.609178016 -0800
+++ b/scripts/checkstack.pl	2010-02-19 22:23:16.038241212 -0800
@@ -21,6 +21,8 @@
 #
 #	TODO :	Port to all architectures (one regex per arch)
 
+use strict;
+
 # check for arch
 #
 # $re is used for two matches:
@@ -104,19 +106,11 @@ my (@stack, $re, $dre, $x, $xs);
 	}
 }
 
-sub bysize($) {
-	my ($asize, $bsize);
-	($asize = $a) =~ s/.*:	*(.*)$/$1/;
-	($bsize = $b) =~ s/.*:	*(.*)$/$1/;
-	$bsize <=> $asize
-}
-
 #
 # main()
 #
 my $funcre = qr/^$x* <(.*)>:$/;
-my $func;
-my $file, $lastslash;
+my ($func, $file, $lastslash);
 
 while (my $line = <STDIN>) {
 	if ($line =~ m/$funcre/) {
@@ -173,4 +167,7 @@ while (my $line = <STDIN>) {
 	}
 }
 
-print sort bysize @stack;
+# Use Schwartzian transform to sort by last field (size)
+print map { $_->[0] }
+      sort { $b->[1] <=> $a->[1] }
+      map { [$_, /:\t*(\d+)$/] } @stack;

Re: [PATCH] scripts: improve checkstack

[Cong Wang]

Stephen Hemminger wrote:
> Turn on strict checking, and get rid of annoying use of prototype.
> Fix syntax error in declaration
> 
> Use efficient sort algorithm by using schwartzian transform.
>   http://en.wikipedia.org/wiki/Schwartzian_transform


Yeah, the idea is good, this can also make it more perlish. ;)

But...


> -print sort bysize @stack;
> +# Use Schwartzian transform to sort by last field (size)
> +print map { $_->[0] }
> +      sort { $b->[1] <=> $a->[1] }
> +      map { [$_, /:\t*(\d+)$/] } @stack;

This regex here is not strictly the same as before.

Can we just keep the original regex? If not, what's wrong?

Thanks.

Re: [PATCH] scripts: improve checkstack

[Stephen Hemminger]

On Sat, 20 Feb 2010 15:35:06 +0800
Cong Wang <amwang@redhat.com> wrote:

> Stephen Hemminger wrote:
> > Turn on strict checking, and get rid of annoying use of prototype.
> > Fix syntax error in declaration
> > 
> > Use efficient sort algorithm by using schwartzian transform.
> >   http://en.wikipedia.org/wiki/Schwartzian_transform
> 
> 
> Yeah, the idea is good, this can also make it more perlish. ;)
> 
> But...
> 
> 
> > -print sort bysize @stack;
> > +# Use Schwartzian transform to sort by last field (size)
> > +print map { $_->[0] }
> > +      sort { $b->[1] <=> $a->[1] }
> > +      map { [$_, /:\t*(\d+)$/] } @stack;
> 
> This regex here is not strictly the same as before.
> 
> Can we just keep the original regex? If not, what's wrong?

The original one had extra cruft:
  1. The expression is anchored on right, so leading .* is meaningless
  2. Putting tab directly makes it invisible when reading source (use \t)
  3. Want to match on number, not just anything (that is why the \d)

Re: [PATCH] scripts: improve checkstack

[Arjan van de Ven]

On 2/22/2010 8:43, Stephen Hemminger wrote:


btw as a general comment on these guys

"make it more perl-ish" is not a good feature for me.

"make it more readable" would be.

these guys are not performance sensitive, lets make them as readable as possible, and not line noise.

Re: [PATCH] scripts: improve checkstack

[Steven Rostedt]

On Mon, 2010-02-22 at 08:58 -0800, Arjan van de Ven wrote:
> On 2/22/2010 8:43, Stephen Hemminger wrote:
> 
> 
> btw as a general comment on these guys
> 
> "make it more perl-ish" is not a good feature for me.
> 
> "make it more readable" would be.
> 
> these guys are not performance sensitive, lets make them as readable as possible, and not line noise.
> 

Acked-by: Steven Rostedt

That is, I've purposely did not do "perlish" code in the perl scripts
that I wrote, and have consistently NACKed changes to make it so.

The perl scripts in the kernel need to be "C-ish" not "perl-ish", that
way the other 70% of the kernel programmers that do not do perl can
still understand these scripts.

-- Steve

Re: [PATCH] scripts: improve checkstack

[Stephen Hemminger]

Cleanup checkstack script:
  * Turn on strict checking
  * Fix resulting error message because the declaration syntax
    was incorrect.
  * Remove incorrect and misleading use of prototype
     - prototype not required for this type of sort function
       because $a and $b are being used in this contex
     - if prototype was being used it should be for both arguments
  * Use closure for sort function

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/scripts/checkstack.pl	2010-02-22 10:15:08.120323236 -0800
+++ b/scripts/checkstack.pl	2010-02-22 10:17:49.878421919 -0800
@@ -21,6 +21,8 @@
 #
 #	TODO :	Port to all architectures (one regex per arch)
 
+use strict;
+
 # check for arch
 #
 # $re is used for two matches:
@@ -104,19 +106,11 @@ my (@stack, $re, $dre, $x, $xs);
 	}
 }
 
-sub bysize($) {
-	my ($asize, $bsize);
-	($asize = $a) =~ s/.*:	*(.*)$/$1/;
-	($bsize = $b) =~ s/.*:	*(.*)$/$1/;
-	$bsize <=> $asize
-}
-
 #
 # main()
 #
 my $funcre = qr/^$x* <(.*)>:$/;
-my $func;
-my $file, $lastslash;
+my ($func, $file, $lastslash);
 
 while (my $line = <STDIN>) {
 	if ($line =~ m/$funcre/) {
@@ -173,4 +167,6 @@ while (my $line = <STDIN>) {
 	}
 }
 
-print sort bysize @stack;
+# Sort output by size (last field)
+print sort { ($b =~ /:\t*(\d+)$/)[0] <=> ($a =~ /:\t*(\d+)$/)[0] } @stack;
+

Re: [PATCH 02/10] checkincludes: fix perlcritic warnings

[Cong Wang]

This looks fine, but your $subject needs to be fixed.

Thanks.

Re: [PATCH 02/10] checkincludes: fix perlcritic warnings

[Stephen Hemminger]

On Sat, 20 Feb 2010 11:05:00 +0800
Cong Wang <amwang@redhat.com> wrote:

> This looks fine, but your $subject needs to be fixed.
> 

Sorry, I use netdev standard subject name format.  (Inherited from Jeff Garzik)
-- 

Re: [PATCH 02/10] checkincludes: fix perlcritic warnings

[Cong Wang]

Stephen Hemminger wrote:
> On Sat, 20 Feb 2010 11:05:00 +0800
> Cong Wang <amwang@redhat.com> wrote:
> 
>> This looks fine, but your $subject needs to be fixed.
>>
> 
> Sorry, I use netdev standard subject name format.  (Inherited from Jeff Garzik)

Oh, sorry, I misunderstood it. Then it is fine.

Re: [PATCH 03/10] Subject checkversion: perl cleanup

[Ralf Baechle]

On Fri, Feb 19, 2010 at 09:52:20AM -0800, Stephen Hemminger wrote:

> Subject: [PATCH 03/10] Subject checkversion: perl cleanup
                         ^^^^^^^^
One Subject too much in that line it seems.

  Ralf

Re: [PATCH 03/10] Subject checkversion: perl cleanup

[Cong Wang]

This one too, please fix the subject.

Re: [PATCH 05/10] profile2linkerlist: fix perl warnings

[Cong Wang]

Acked-by: WANG Cong <amwang@redhat.com>

Re: [PATCH 06/10] export_report: fix perl warnings

[Cong Wang]

Except you have some indention fix, the patch looks fine for me.

Acked-by: WANG Cong <amwang@redhat.com>

Thanks.

Re: [PATCH 07/10] headers_check: fix perl warnings

[Cong Wang]

Acked-by: WANG Cong <amwang@redhat.com>

Thanks.

Re: [PATCH 08/10] headers_install: use local file handles

[Cong Wang]

This looks fine for me.

Acked-by: WANG Cong <amwang@redhat.com>

Thanks.

Re: [PATCH 09/10] markup_oops: fix perlcritic warnings

[Cong Wang]

Acked-by: WANG Cong <amwang@redhat.com>

Thanks.

Re: [PATCH 10/10] headerdep: perlcritic warning

[Cong Wang]

Acked-by: WANG Cong <amwang@redhat.com>

Thanks.

Re: [PATCH 04/10] namespace: perlcritic warnings

[Cong Wang]

You do more than what you said, e.g. you replaced a return in a
subroutine with die() which I think is not correct.

Please describe every of your changes.

Thanks.

Re: [PATCH 04/10] namespace: perlcritic warnings

[Stephen Hemminger]

If objdump program is missing program should die with message
to standard error and exit with non zero status, not continue on!

Re: [PATCH 04/10] namespace: perlcritic warnings

[Cong Wang]

Stephen Hemminger wrote:
> If objdump program is missing program should die with message
> to standard error and exit with non zero status, not continue on!

Oh, right, makes sense.

Re: [PATCH 04/10] namespace: perlcritic warnings

[Hui Zhu]

On Sat, Feb 20, 2010 at 01:52, Stephen Hemminger <shemminger@vyatta.com> wrote:> Use local file handle not global.> Make loop and other variables local in scope.>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>>>> --- a/scripts/namespace.pl      2010-02-19 08:59:00.432184740 -0800> +++ b/scripts/namespace.pl      2010-02-19 09:05:52.672644797 -0800> @@ -175,12 +175,11 @@ sub do_nm>        }>        if (! -e "$source.c" && ! -e "$source.S") {>                # No obvious source, exclude the object if it is conglomerate> -               if (! open(OBJDUMPDATA, "$objdump $basename|")) {> -                       printf STDERR "$objdump $fullname failed $!\n";> -                       return;> -               }> +               open(my $objdumpdata, "$objdump $basename|")> +                   or die "$objdump $fullname failed $!\n";
Looks it just want return, are you sure have to change it to die?
> +>                my $comment;> -               while (<OBJDUMPDATA>) {> +               while (<$objdumpdata>) {>                        chomp();>                        if (/^In archive/) {>                                # Archives are always conglomerate> @@ -190,18 +189,18 @@ sub do_nm>                        next if (! /^[ 0-9a-f]{5,} /);>                        $comment .= substr($_, 43);>                }> -               close(OBJDUMPDATA);> +               close($objdumpdata);> +>                if (!defined($comment) || $comment !~ /GCC\:.*GCC\:/m) {>                        printf STDERR "No source file found for $fullname\n";>                }>                return;>        }> -       if (! open(NMDATA, "$nm $basename|")) {> -               printf STDERR "$nm $fullname failed $!\n";> -               return;> -       }> +       open (my $nmdata, "$nm $basename|")> +           or die "$nm $fullname failed $!\n";> +>        my @nmdata;> -       while (<NMDATA>) {> +       while (<$nmdata>) {>                chop;>                ($type, $name) = (split(/ +/, $_, 3))[1..2];>                # Expected types> @@ -268,7 +267,8 @@ sub do_nm>                        }>                }>        }> -       close(NMDATA);> +       close($nmdata);> +>        if ($#nmdata < 0) {>                if (>                        $fullname ne "lib/brlock.o"> @@ -316,8 +316,7 @@ sub drop_def>>  sub list_multiply_defined>  {> -       my ($name, $module);> -       foreach $name (keys(%def)) {> +       foreach my $name (keys(%def)) {>                if ($#{$def{$name}} > 0) {>                        # Special case for cond_syscall>                        if ($#{$def{$name}} == 1 && $name =~ /^sys_/ &&> @@ -333,8 +332,9 @@ sub list_multiply_defined>                                &drop_def("arch/x86/kernel/vsyscall-sysenter_32.o", $name);>                                next;>                        }> +>                        printf "$name is multiply defined in :-\n";> -                       foreach $module (@{$def{$name}}) {> +                       foreach my $module (@{$def{$name}}) {>                                printf "\t$module\n";>                        }>                }> @@ -343,12 +343,13 @@ sub list_multiply_defined>>  sub resolve_external_references>  {> -       my ($object, $type, $name, $i, $j, $kstrtab, $ksymtab, $export);> +       my ($kstrtab, $ksymtab, $export);> +>        printf "\n";> -       foreach $object (keys(%nmdata)) {> +       foreach my $object (keys(%nmdata)) {>                my $nmdata = $nmdata{$object};> -               for ($i = 0; $i <= $#{$nmdata}; ++$i) {> -                       ($type, $name) = split(' ', $nmdata->[$i], 2);> +               for (my $i = 0; $i <= $#{$nmdata}; ++$i) {> +                       my ($type, $name) = split(' ', $nmdata->[$i], 2);>                        if ($type eq "U" || $type eq "w") {>                                if (exists($def{$name}) || exists($ksymtab{$name})) {>                                        # add the owning object to the nmdata> @@ -357,7 +358,7 @@ sub resolve_external_references>                                        $kstrtab = "R __kstrtab_$name";>                                        $ksymtab = "R __ksymtab_$name";>                                        $export = 0;> -                                       for ($j = 0; $j <= $#{$nmdata}; ++$j) {> +                                       for (my $j = 0; $j <= $#{$nmdata}; ++$j) {>                                                if ($nmdata->[$j] eq $kstrtab ||>                                                    $nmdata->[$j] eq $ksymtab) {>                                                        $export = 1;> @@ -424,11 +425,11 @@ sub resolve_external_references>  sub list_extra_externals>  {>        my %noref = ();> -       my ($name, @module, $module, $export);> -       foreach $name (keys(%def)) {> +> +       foreach my $name (keys(%def)) {>                if (! exists($ref{$name})) {> -                       @module = @{$def{$name}};> -                       foreach $module (@module) {> +                       my @module = @{$def{$name}};> +                       foreach my $module (@module) {>                                if (! exists($noref{$module})) {>                                        $noref{$module} = [];>                                }> @@ -438,16 +439,16 @@ sub list_extra_externals>        }>        if (%noref) {>                printf "\nExternally defined symbols with no external references\n";> -               foreach $module (sort(keys(%noref))) {> +               foreach my $module (sort(keys(%noref))) {>                        printf "  $module\n";>                        foreach (sort(@{$noref{$module}})) {> -                               if (exists($export{$_})) {> -                                       $export = " (export only)";> -                               }> -                               else {> -                                       $export = "";> -                               }> -                               printf "    $_$export\n";> +                           my $export;> +                           if (exists($export{$_})) {> +                               $export = " (export only)";> +                           } else {> +                               $export = "";> +                           }> +                           printf "    $_$export\n";>                        }>                }>        }>> -->>ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 04/10] namespace: perlcritic warnings

[Stephen Hemminger]

On Sat, 20 Feb 2010 16:26:24 +0800
Hui Zhu <teawater@gmail.com> wrote:

> On Sat, Feb 20, 2010 at 01:52, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > Use local file handle not global.
> > Make loop and other variables local in scope.
> >
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> >
> > --- a/scripts/namespace.pl      2010-02-19 08:59:00.432184740 -0800
> > +++ b/scripts/namespace.pl      2010-02-19 09:05:52.672644797 -0800
> > @@ -175,12 +175,11 @@ sub do_nm
> >        }
> >        if (! -e "$source.c" && ! -e "$source.S") {
> >                # No obvious source, exclude the object if it is conglomerate
> > -               if (! open(OBJDUMPDATA, "$objdump $basename|")) {
> > -                       printf STDERR "$objdump $fullname failed $!\n";
> > -                       return;
> > -               }
> > +               open(my $objdumpdata, "$objdump $basename|")
> > +                   or die "$objdump $fullname failed $!\n";
> 
> Looks it just want return, are you sure have to change it to die?

Read my previous comment 
> If objdump program is missing program should die with message
> to standard error and exit with non zero status, not continue on!