[Webkit-unassigned] [Bug 25517] build-webkit script should print build time at end

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25517


David Kilzer (ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #38923|review?                     |review-
               Flag|                            |




--- Comment #5 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2009-09-02 07:10:19 PDT ---
(From update of attachment 38923)
The ChangeLog looks good!  I think one more patch should do it.  Most of the
issues are simply following coding style used in other Perl scripts in the
WebKit project.  (Sorry, I missed some of these on the first review.)

> +my $startTime = time;

Since 'time' here is a subroutine call, please add empty parenthesis:  time()

> +# Formatting routine to display build time

The comment isn't necessary; I think the name of the subroutine is
self-explanatory.

> +sub formatBuildTime
> +{

New subroutines added to Perl script should have the number of parameters
declared:

sub formatBuildTime($)
{

The subroutine itself should appear (alphabetically--although this would be the
first one) after the  last "exit 0;" statement in the file.

Also, this subroutine should be declared at the top of the script with its
parameter list (it should come after the 'use' statements but before any global
variables):

 use POSIX;

+sub formatBuildTime($);

 my $originalWorkingDirectory = getcwd();


> +    my $buildTime = $_[0];

This is normally written using the @_ array in other Perl scripts:

    my ($buildTime) = @_;

> +    my $buildHours = ($buildTime - $buildTime % 3600) / 3600;

This would be easier to read using:

    my $buildHours = int($buildTime / 3600);

> +    my $buildMins = (($buildTime - $buildHours * 3600) - ($buildTime - $buildHours*3600) % 60) / 60;

Similarly:

    my $buildMins = int(($buildTime - $buildHours * 3600) / 60)

> +    if ($buildHours) {
> +        my $result = $buildHours,':',$buildMins,':',$buildSecs;
> +    } else {
> +        my $result = $buildMins,':',$buildSecs;
> +    }

A few items here:

- WebKit usually prefers to use an "early return" instead of an if/else block
in these cases.
- WebKit prefers explicit return statements for values.  This code uses
implicit return values, e.g., the last statement executed, and declares a
$result variable that's never really used.
- The formatBuildTime() subroutine should really return a string instead of a
list of strings.
- The minutes and seconds values should be zero-padded so you have times like
"1:01:05" instead of "1:1:5".
- I think it would be nice to add "h", "m" and "s" suffixes to the time values
to make them easier to read:  "18m:02s", "1h:01m:05s".

Here's how that code might look:

    if ($buildHours) {
        return sprintf("%dh:%02dm:%02ds", $buildHours, $buildMins, $buildSecs);
    }
    return sprintf("%02dm:%02ds", $buildMins, $buildSecs);

> +my $endTime = time;

This should use "time()" since it's a subroutine call.

> -print " WebKit is now built. To run $launcherName with this newly-built\n";
> -print " code, use the \"$launcherPath\" script.\n";
> +print " WebKit is now built (", &formatBuildTime($endTime - $startTime), "). \n";
> +print " To run $launcherName with this newly-built code, use the\n";
> +print " \"$launcherPath\" script.\n";

Instead of calling the subroutine inside the print statement, let's set a
variable outside the "print" statements (no need to use the '&' modifier here
to invoke the subroutine):

my $buildTime = formatBuildTime($endTime - $startTime);

Then we can just use the variable in the print statement (like the others
already do):

> +print " WebKit is now built ($buildTime). \n";

r- for the remaining issues.  Thanks!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.