Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl

On Sun, 3 May 2009 23:34:03 +0200, Francis Galiegue [off-list ref]
wrote:

Le Sunday 03 May 2009 22:58:26 Jeff King, vous avez écrit :

quoted

On Wed, Apr 29, 2009 at 09:12:23AM -0400, Bill Pemberton wrote:

quoted

The script was using bareword filehandles.  This is considered a bad
practice so they have been changed to indirect filehandles.

I think this is a real improvement; using indirect filehandles mean they
get scoped properly, which can avoid errors (especially forgetting to
close() them, which happens automagically when they go out of scope).
Assuming, of course, that the scoping added by your change is correct,
and doesn't close a handle during a loop that we may have wanted to keep
open (I didn't check carefully).

But in the patch itself:

quoted

-	open(C,">",$compose_filename)
+	open my $C,'>',$compose_filename

There are actually two things happening here:

  1. s/C/my $C/, which I think is good

  2. losing the parentheses around open(). This is a style issue, but I
     think we usually prefer the parenthesized form of most perl
     builtins (and certainly in the absence of other information, it
     should be left as-is).

And why not go the full way and using IO::File?

Because that would be travelling back in time.
The most efficient and preferred way is three-arg lexical:

    open my $fh, "<", $filename or die "$filename: $!";
    while (<$fh>) {
        # ...
        }
    close $fh or die "$filename: $!";

give or take quoting style, some spaces and/or indents

my $fh = new IO::File;

$fh->open("/the/file", O_RDONLY|...)

Why use a module for something that is neatly buit in?

-- 
H.Merijn Brand  http://tux.nl      Perl Monger  http://amsterdam.pm.org/
using & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,
11.11, 11.23, and 11.31, OpenSuSE 10.3, 11.0, and 11.1, AIX 5.2 and 5.3.
http://mirrors.develooper.com/hpux/           http://www.test-smoke.org/
http://qa.perl.org      http://www.goldmark.org/jeff/stupid-disclaimers/