From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Thu, 30 Sep 2010 16:33:59 GMT
Subject: Re: [PATCH 13/16] send-email: extract_valid_address use qr// regexes and /o
Message-ID: <AANLkTimSHxU8EhuOHn77kw+ASCCeg_Bjg2or7L=G_o=M@mail.gmail.com>
In-Reply-To: <20100930161912.GA8707@sigill.intra.peff.net>

On Thu, Sep 30, 2010 at 16:19, Jeff King <peff@peff.net> wrote:
> 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.

IIRC different perl versions treat this differently. In more recent
versions doing:

    perl -Mre=debug -E 'sub x { my $x = qr/foo/; my $y = qr/bar/;
/$x$y/ } x($_) for 1..2'

Will only compile all of those regexes once, since perl can see that
they're constant. So the /o does nothing.

We might want to keep it for self-documentation purposes to indicate
that the $local_part_regexp would never change, but it's probably best
to drop that /o altogether.

> 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/.../;
>      ...
>    }
>  }

Perl has static variables like that, just not in the ancient version
we're pinned to:

    sub foo {
        state $foo = qr/$_[0]/
    }