Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o

From: Jeff King <peff@peff.net>
Date: Sep 30, 2010, 16:19 UTC
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:

Show 11 quoted lines

> 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