Re: [RFC PATCH 2/3] export symbol report: export-symbol usage report generator.

[ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)]

linuxram@us.ibm.com (Ram Pai) writes:

> The following patch provides the ability to generate a report of
>      (1) All the exported symbols and their in-kernel-module usage count 
>      (2) For each module, lists the modules and their exported symbols, on
> 		which it depends.

Neat. Just a few Perl nits (sorry, couldn't resist).

> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
[...]
> Index: 2617rc1/scripts/export_report.pl
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ 2617rc1/scripts/export_report.pl	2006-04-18 16:02:32.000000000 -0700
> @@ -0,0 +1,134 @@
[...]
> +sub alphabetically {
> +	($module1, $value1, undef) = split / /, "@{$a}";
> +	($module2, $value2, undef) = split / /, "@{$b}";

This:

> +	if ($value1 == $value2) {
> +		if ($module1 lt $module2) {
> +			return 1;
> +		} elsif ($module1 eq $module2) {
> +			return 0;
> +		} 
> +		return -1;
> +	}
> +	return $value1 <=> $value2;

can be written more idiomatically (and readably, IMHO):

        return $value1 <=> $value2 || $module1 cmp $module2;
> +}
[...]
> +#
> +# collect the usage count of each symbol.
> +#

And this:

> +for ($i = 0; $i <= $#ARGV; $i++) {
> +	$thismod = $ARGV[$i];

could be:

  foreach $thismod (@ARGV) {

> +	unless (open(MODULE_MODULE, $thismod)) {
> +		print "Sorry, cannot open $kernel: $!\n";
> +		next;
> +	}
> +	while ( <MODULE_MODULE> ) {
> +		chomp;
> +		if ( $_ !~ /0x[0-9a-f]{7,8},/ ) {
> +			next;
> +		}

Instead of this:

> +		(undef, undef, undef, undef, $symbol) = split /([,"])/, $_;

some may prefer:

                $symbol = (split /([,"])/)[4];

> +		($module, $value, $symbol, $gpl) = @{$SYMBOL{$symbol}};
> +		$SYMBOL{ $symbol } =  [ $module , $value+1 , $symbol, $gpl];
> +		push(@{$MODULE{$thismod}} , $symbol);
> +	}
> +	close(MODULE_MODULE);
> +}
[...]
> +while (($thismod, $list) = each %MODULE) {
> +	undef %depends;

If you instead use

        my %depends;

the variable will be lexically scoped to the body of the loop and thus
undef at the start of each iteration. I notice the lack of 'use strict'
and lexically scoped variables throughout this script, which makes it
less readable and maintainable than possible, IMHO.

> +	print "\t\t\t$thismod\n";
> +	foreach $symbol (@{$list}) {
> +		($module, $value, undef, $gpl) = @{$SYMBOL{$symbol}};
> +		push (@{$depends{"$module"}}, "$symbol $value");
> +	}
> +	print_depends_on(\%depends);
> +}

-- 
ilmari
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen