doctrine / lexer

Base library for a lexer that can be used in Top-Down, Recursive Descent Parsers.
https://www.doctrine-project.org/projects/lexer.html
MIT License
11.07k stars 60 forks source link

Extract static variable $regex into property #13

Closed kstkn closed 5 years ago

kstkn commented 6 years ago

Method \Doctrine\Common\Lexer\AbstractLexer::scan contains static variable $regex, which shares state between class instances. Consider following example, in which second instance ($lexer2) has different catchable pattern ([a-z]+), but still behaves like the first one:

class Lexer extends \Doctrine\Common\Lexer\AbstractLexer
{
    private $catchablePatterns = [];

    public function addCatchablePattern($pattern)
    {
        $this->catchablePatterns[] = $pattern;
    }

    protected function getCatchablePatterns()
    {
        return $this->catchablePatterns;
    }

    protected function getNonCatchablePatterns()
    {
        return ['[\s,]+'];
    }

    protected function getType(&$value)
    {
        return 1;
    }
}

$lexer1 = new Lexer();
$lexer1->addCatchablePattern('[a-z]');
$lexer1->setInput('one');
$token = $lexer1->glimpse();
var_dump($token['value']);
// string(1) "o"

$lexer2 = new Lexer();
$lexer2->addCatchablePattern('[a-z]+');
$lexer2->setInput('one');
$token = $lexer2->glimpse();
var_dump($token['value']);
// string(1) "o"
jwage commented 6 years ago

This makes sense. Unless there is some valid reason why it was done this way. For performance or something, but I doubt it. Can you add a unit test?

kstkn commented 6 years ago

@jwage test added

jwage commented 6 years ago

@guilhermeblanco can you take a look at this?

guilhermeblanco commented 6 years ago

The static was added exactly for performance reasons, but can be addressed by using the private variable, for sure. We did that before because we could b using the same Lexer for multiple queries and didn't want to rebuild the regex every time (as Lexer in ORM is not shared).

guilhermeblanco commented 6 years ago

Anyway, LGTM

kstkn commented 6 years ago

@jwage