Stabilizing Jison in Php

[robertleeplummerjr]

No description provided.

[zaach]

@robertleeplummerjr Did you have a look at #195? Were any of those commits useful?

[robertleeplummerjr]

Sure. Merge it in.

On Tue, Jun 24, 2014 at 2:04 PM, Zach Carter notifications@github.com
wrote:

@robertleeplummerjr https://github.com/robertleeplummerjr Did you have
a look at #195 #195? Were any of
those commits useful?

—
Reply to this email directly or view it on GitHub
#220 (comment).

Robert Plummer

[robertleeplummerjr]

Upstreamed

[damienalexandre]

There is a mix of tabs and spaces, you should follow the project standards.

[robertleeplummerjr]

That is actually a really good point. I only use tabs, must be visual
studio. I'll do a cleanup later today.
On Sep 22, 2014 6:17 AM, "Damien Alexandre" notifications@github.com
wrote:

In ports/csharp/Jison/Jison/Template.cs:

        return ch;
    }

    public void Unput(string ch)
    {

•        var yy = new /**/ParserValue/**/();
  

There is a mix of tabs and spaces, you should follow the project standards.

—
Reply to this email directly or view it on GitHub
https://github.com/zaach/jison/pull/220/files#r17841300.

[robertleeplummerjr]

And fixed. Thanks for spotting!

[robertleeplummerjr]

Php unit tests in wikiLingo all powered by Jison in PHP: http://wikilingodoesthat.com/demo/test.php

[zaach]

This changes the signature of the parse function, which i'm weary of doing lightly. It will break consumer code. As for this particular change, returning an error string seems like the wrong API. Errors are already thrown or handle by a custom error handler. Let's get rid of this change.

[zaach]

@robertleeplummerjr Besides the one issue I commented on, this is good to go.

[robertleeplummerjr]

This simply allows the parser to handle errors rather than throwing them.
But notice you must create a method that overrides the existing error
method for this to do anything, otherwise the functionality is the same as
it was before. The reason is that the code just before this executes,
because this.done equals false. If something in the override sets
this.done to true, then the parser returns the error. Something to note as
well: throwing an error only to catch is expensive, and is not industry
standard, especially if you are using jison within thousands of parses in a
short amount of time. Thus, this can be seen as a performance enhancement
and a means of further tailoring the parser to any implementation.
Standard bison allows type of logic, this actually too brings jison closer
to how bison operates.

@robertleeplummerjr https://github.com/robertleeplummerjr Besides the one
issue I commented on, this is good to go.

—
Reply to this email directly or view it on GitHub
#220 (comment).

[zaach]

Performance is a valiant goal, but my problems with the implementation are that it changes the parser API for a special case that relies on parser state (this.done), preventing the parser from being used recursively. Effort has been made to ensure that is not the case– we shouldn't add more of it.

Another approach would be to add a parser option to return the value from parseError. The default parseError behavior could be to return the error message instead of throwing. Custom parseError could also make parse return immediately if it returns a value.

But this change has nothing to do with the PHP port, so it needs to go in a separate PR in any case.

[robertleeplummerjr]

I will move it to another PR, but to your point that it prevents the parser
from being used recursively:
The method "setInput", which is called before any parse has the following
line:
this._more = this._backtrack = this.done = false;

"setInput" is called any time the parser is called to parse a string. So
"my problems with the implementation are that it changes the parser API for
a special case" is inaccurate, as it does not change the api for a special
use case, but rather allows it to be extended. Setting "done" to true is
only good for the parse that is happening when it is set, and will not
affect things recursively. I thought you would know that, as I believe you
wrote that line. I do, however, like your approach on allowing there to be
an option to return the value from parseError. But how would you add that
option? Wouldn't that change the api as well?

On Sun, Oct 19, 2014 at 1:29 PM, Zach Carter notifications@github.com
wrote:

Performance is a valiant goal, but my problems with the implementation are
that it changes the parser API for a special case that relies on parser
state (this.done), preventing the parser from being used recursively.
Effort has been made to ensure that is not the case– we shouldn't add
more of it.

Another approach would be to add a parser option to return the value from
parseError. The default parseError behavior could be to return the error
message instead of throwing. Custom parseError could also make parse
return immediately if it returns a value.

But this change has nothing to do with the PHP port, so it needs to go in
a separate PR in any case.

—
Reply to this email directly or view it on GitHub
#220 (comment).

Robert Plummer

[zaach]

The method "setInput", which is called before any parse has the following
line:
this._more = this._backtrack = this.done = false;

Ah, so it looks like there's a bug in your patch which is causing my confusion. this refers to the lexer within setInput– not the parser. Your patch would need to use lexer.done within parse for it to work, (which introduces more coupling between the lexer and parser). So yes, setting state on the lexer is reentrant, but setting it on the parser is not.

as it does not change the api for a special use case, but rather allows it to be extended.

Okay, it "extends" the api for a special use case. I'd rather do something more general and more explicit and let users declare up front if they want to change the behavior of error handling in the parser. It also avoids the awkward coupling between the lexer and parser.

But how would you add that option? Wouldn't that change the api as well?

It would be a declaration in the grammar file, like other options. It will change the way the parser is generated.

[robertleeplummerjr]

Thanks for the correction, I see the bug, my bad. Rolled everything back
in jison.js to your most reset commit. Thanks again Zach.

On Sun, Oct 19, 2014 at 3:15 PM, Zach Carter notifications@github.com
wrote:

The method "setInput", which is called before any parse has the following
line:
this._more = this._backtrack = this.done = false;

Ah, so it looks like there's a bug in your patch which is causing my
confusion. this refers to the lexer within setInput– not the parser. Your
patch would need to use lexer.done within parse for it to work, (which
introduces more coupling between the lexer and parser). So yes, setting
state on the lexer is reentrant, but setting it on the parser is not.

as it does not change the api for a special use case, but rather allows it
to be extended.
Okay, it "extends" the api for a special use case. I'd rather do something
more general and more explicit and let users declare up front if they want
to change the behavior of error handling in the parser. It also avoids the
awkward coupling between the lexer and parser.

But how would you add that option? Wouldn't that change the api as well?
It would be a declaration in the grammar file, like other options. It will
change the way the parser is generated.

—
Reply to this email directly or view it on GitHub
#220 (comment).

Robert Plummer

[robertleeplummerjr]

robertleeplummerjr@3d35ae6

On Mon, Oct 20, 2014 at 10:00 AM, Robert Plummer <
robertleeplummerjr@gmail.com> wrote:

Thanks for the correction, I see the bug, my bad. Rolled everything back
in jison.js to your most reset commit. Thanks again Zach.

On Sun, Oct 19, 2014 at 3:15 PM, Zach Carter notifications@github.com
wrote:

The method "setInput", which is called before any parse has the following
line:
this._more = this._backtrack = this.done = false;

Ah, so it looks like there's a bug in your patch which is causing my
confusion. this refers to the lexer within setInput– not the parser.
Your patch would need to use lexer.done within parse for it to work,
(which introduces more coupling between the lexer and parser). So yes,
setting state on the lexer is reentrant, but setting it on the parser is
not.

as it does not change the api for a special use case, but rather allows
it to be extended.
Okay, it "extends" the api for a special use case. I'd rather do
something more general and more explicit and let users declare up front if
they want to change the behavior of error handling in the parser. It also
avoids the awkward coupling between the lexer and parser.

But how would you add that option? Wouldn't that change the api as well?
It would be a declaration in the grammar file, like other options. It
will change the way the parser is generated.

—
Reply to this email directly or view it on GitHub
#220 (comment).

Robert Plummer

Robert Plummer

[robertleeplummerjr]

Zach is apparently busy with other projects. This will end our relationship with me contributing to Jison for the foreseeable future. I will now put effort into my own branch.