From: Jeff King <peff@peff.net>
Date: Thu, 30 Sep 2010 16:19:12 GMT
Subject: Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o
Message-ID: <20100930161912.GA8707@sigill.intra.peff.net>
In-Reply-To: <1285854189-10240-14-git-send-email-avarab@gmail.com>

On Thu, Sep 30, 2010 at 01:43:06PM +0000, Ævar Arnfjörð Bjarmason wrote:

> Change the regex fragment in extract_valid_address to use the qr//
> syntax for compiled regexes, and when they're used add a /o flag so
> they're only compiled once for the lifetime of the program.
> [...]
>  sub extract_valid_address {
>  	my $address = shift;
> -	my $local_part_regexp = '[^<>"\s@]+';
> -	my $domain_regexp = '[^.<>"\s@]+(?:\.[^.<>"\s@]+)+';
> +	my $local_part_regexp = qr/[^<>"\s@]+/;
> +	my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
>  

Hmm. But these are lexical variables, so won't we recompile them each
time we enter the subroutine? I don't think it affects correctness, as
this "/o":

> +	return $address if ($address =~ /^($local_part_regexp)$/o);

means that we will compile and use the value from the first time we run
the function.

But we are unnecessarily compiling the sub-regexes each time. Not that
this is probably a performance critical piece of code, but your "/o" is
doing very little, and this is exactly the sort perl wankery that I find
interesting.

Sadly, there is no real perl equivalent of C static local variables,
which is what you really want.  Usually I would do:

  {
    my $local_part_regexp = qr/.../;
    sub extract_valid_address {
      ...
    }
  }

but beware of the execution order. That works well in a module, where
the module code is executed before anybody calls the function. But it
breaks in something like this:

  foo();
  {
    my $foo_static_local = 5;
    sub foo {
      print "$foo_static_local\n";
    }
  }

I think you could get by with:

  {
    my $local_part_regexp;
    sub extract_valid_address {
      $local_part_regexp ||= qr/.../;
      ...
    }
  }

-Peff