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

From: Jeff King <peff@peff.net>
Date: Oct 1, 2010, 05:40 UTC
Message-ID: <20101001054029.GA18674@sigill.intra.peff.net>
In-Reply-To: <AANLkTimSHxU8EhuOHn77kw+ASCCeg_Bjg2or7L=G_o=M@mail.gmail.com>

On Thu, Sep 30, 2010 at 04:33:59PM +0000, Ævar Arnfjörð Bjarmason wrote:

Show 10 quoted lines

> > 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'

Ah, right. I am used to using qr// with variable input (since that is the intended use), but of course perl is smart enough to just need to compile qr/constant/ once.

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

Nice. I didn't think it would be smart enough to handle the layer of indirection, but obviously it does. Go perl.

> 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.

Yeah, I would just drop the /o. It will in all probability never change, but it it _did_, I think having the /o just makes it buggy.

Show 6 quoted lines

> Perl has static variables like that, just not in the ancient version
> we're pinned to:
> 
>     sub foo {
>         state $foo = qr/$_[0]/
>     }

Nice, I obviously have not kept up on my perl-changelog-reading. ;) But yeah, it's not in 5.8. It's a moot point anyway, since as you demonstrated above, perl is already doing the optimization.

-Peff