Getter method for "action"s for Scanless::G grammars

[dginev]

I was wondering if it makes sense to have a "rule_action" method for the Scanless::G grammars, that can fetch the "action" subroutine from a rule/token inside a SLIF grammar.

Something on the lines of:

my $semantic_routine = $grammar->rule_action($rule_id);

A method of such sort would help us to keep writing SLIF grammar and yet use the ASF traversal to do semantics.

It is probably a great way to continue writing a single grammar that solves our task, yet be able to utilize all traversal strategies of the recognizer.

[jeffreykegler]

Good idea.

[jeffreykegler]

This will require some thinking, and probably will have to wait until the parse engine rewrite is complete.

[jeffreykegler]

Getting a head start on the thinking, there are some conceptual issues. Where the action is a Perl closure, what is wanted is pretty clear. But actions can be builtin's like ::first. Worse they can be square bracketed descriptors, like [start,length,value] which are currently implemented in XS. Neither of these correspond conveniently to anything a method could return.

[dginev]

Why not return undef for any case that is "too special"? My grammar has exclusively Perl closures for the semantics, and having a method which only gets those (and returns undef otherwise) would still solve my problem entirely.

Also, if they are implemented in XS, couldn't you return the name of the XS subrtouine that deals with them? If I remember correctly, XS subroutines are also accessible in Perl.

[jeffreykegler]

I could call it rule_action_closure() and return undef if it's not a closure.

The XS logic is mainly in case sections of C statements. And XS subroutines need to be wrapped correctly in order to be seen from Perl -- there are two types wrapped, and unwrapped. The wrapped ones are available to Perl, but not within XS, and the unwrapped ones within XS but not to Perl code. You can wrap an unwrapped subroutine, but since XS is basically all about wrapping, that almost amounts to a re-implementation.

[dginev]

rule_action_closure() sounds like a great first stab at it. That would give me a bit of leeway to try and see what other walls I hit while doing semantics (I gave up on trying to find a workaround to proceeding without this method, since the internals scared me away... sorry about that)

[jeffreykegler]

I looked at the code for this and even did a fair amount of refactoring -- the code to determine the semantics is mixed in with the SLR's implementation of them, and the two should be pulled apart. With that I might be able to have rule_closure() always return a Perl closure -- either the one you specified or one dummied up to provide the semantics you asked for.

If the refactoring had proved easier, I would have snuck this one into the Phase 1/2 interface developer's version, just uploaded. But there'll be another inter-phase after this rewrite phase, and I'll pick up where I left off.

[rns]

Jeffrey, Deyan, can you take a look at this PR? I did the first cut at rule_closure() and would like to know if I got it right.

[jeffreykegler]

rns: if you want to tackle $slr->rule_closure(), that'd be great. I've already done some refactoring, so it's a bit like the joke about locksmith prices: "Fixing locks: $10: If you tried to fix it first: $20'.

Also, because I'm now in phase 2, I was postponing work on these things until the next inter-phase. That may be just as well, because it probably means they'll be no rival changes to that section of code to cause merge issues. I will try to keep up with your questions, though, even before the inter-phase.

My current idea was to catch the Perl closures (note this means getting all the defaults, etc., etc., which was a major aspect of my refactoring so far.), and to dummy up the other kinds of action (::first, [value], etc.) in Perl code.

[rns]

Well, I think this would save you some time for important things only you can do, so I'll try.

I did the first implementation and a test; will you please take a look when you have time.

For now it just calls the registration initialization code refactored out of value() into a sub and returns $recce->[Marpa::R2::Internal::Recognizer::CLOSURE_BY_RULE_ID]->[$rule_id]

[jeffreykegler]

I looked it over quickly. I take it you're planning of going further steps that make rule_closure() smarter?

I greatly appreciate your taking this on. It's important also -- the difference between things like this and Libmarpa internals is that for the moment only I can do those. One hope for the rewrite is that Libmarpa will be greatly simplfied -- one-third or more of the code eliminated, the part eliminated being most obscure third. With that others will be able to delve in.

[rns]

Now it seems to do what Deyan asked. We can leave it at that for the moment and look for the feedback.

ASF already provides the functionality of start, length, lhs, values builtins with glade_span(), symbol_id() and rh_length()/rh_value() (and rh_values()), so if the idea is to allow ASF traversers to call some or all of the actions specified in the grammar, they can do that with the current version of rule_closure().

[jeffreykegler]

@rns : Sounds good.

Re the new "lhs" descriptor element -- could you add it to the POD? The others we should perhaps leave as experimental and undocumented for the moment.

[dginev]

I would gladly try Ruslan's enhancement, if you point me to instructions on installing Marpa::R2 from source (does the casual perl Makefile.PL; make ;make test; make install work?)

[rns]

On Tue, Jan 21, 2014 at 10:58 PM, Jeffrey Kegler notifications@github.comwrote:

@rns https://github.com/rns : Sounds good.

Re the new "lhs" descriptor element -- could you add it to the POD?

Sure, will give it a shot tomorrow.

The others we should perhaps leave as experimental and undocumented for the moment.

Yes.

— Reply to this email directly or view it on GitHubhttps://github.com/jeffreykegler/Marpa--R2/issues/78#issuecomment-32962520 .

[rns]

On Tue, Jan 21, 2014 at 10:59 PM, Deyan Ginev notifications@github.comwrote:

I would gladly try Ruslan's enhancement, if you point me to instructions on installing Marpa::R2 from source (does the casual perl Makefile.PL; make ;make test; make install work?)

Yes, Marpa::R2 uses Module::Build: perl Build.PL; ./Build; ./Build test; ./Build install.

But to build the dist tarball from the repo you need cweb and tex live and fonts, so I'd better email you the tarball (~1.8 Mb) build on the repo branch with rule_closure() and rh_values() to your gmail address I've seen on the mailing list or any other.

— Reply to this email directly or view it on GitHubhttps://github.com/jeffreykegler/Marpa--R2/issues/78#issuecomment-32962631 .

[dginev]

Don't mail me anything please :) We should have a way of doing things through GitHub. I don't mind installing the dependencies (I think I actually have them already). Will give it a spin now.

[rns]

On Tue, Jan 21, 2014 at 11:29 PM, Deyan Ginev notifications@github.comwrote:

Don't mail me anything please :)

Ahh, sure. :))

We should have a way of doing things through GitHub. I don't mind installing the dependencies (I think I actually have them already). Will give it a spin now.

Great, rule_closure() and rh_values() are in this branchhttps://github.com/rns/Marpa--R2/commits/rule_closure.

— Reply to this email directly or view it on GitHubhttps://github.com/jeffreykegler/Marpa--R2/issues/78#issuecomment-32965564 .

[jddurand]

Le mardi 21 janvier 2014, 13:27:56 rns a écrit :

On Tue, Jan 21, 2014 at 10:59 PM, Deyan Ginev notifications@github.comwrote: Yes, Marpa::R2 uses Module::Build: perl Build.PL; ./Build; ./Build test; ./Build install.

But to build the dist tarball from the repo you need cweb and tex live and fonts, so I'd better email you the tarball (~1.8 Mb) build on the repo branch with rule_closure() and rh_values() to your gmail address I've seen on the mailing list or any other.

Could you remind me what command is typed for build the dist tarball. Thanks / JD.

[rns]

Sure, make releng.

On Tue, Jan 21, 2014 at 11:42 PM, jddurand notifications@github.com wrote:

Le mardi 21 janvier 2014, 13:27:56 rns a écrit :

On Tue, Jan 21, 2014 at 10:59 PM, Deyan Ginev notifications@github.comwrote:

Yes, Marpa::R2 uses Module::Build: perl Build.PL; ./Build; ./Build test; ./Build install.

But to build the dist tarball from the repo you need cweb and tex live and fonts, so I'd better email you the tarball (~1.8 Mb) build on the repo branch with rule_closure() and rh_values() to your gmail address I've seen on the mailing list or any other.

Could you remind me what command is typed for build the dist tarball. Thanks / JD.

— Reply to this email directly or view it on GitHubhttps://github.com/jeffreykegler/Marpa--R2/issues/78#issuecomment-32966816 .

[dginev]

@rns if you can join IRC, that would help me a bit. As it stands, I am missing an expected cpan/libmarpa directory in the repository, maybe I need to do some sort of recursive checkout or.. ?

EDIT: Ah, it's a recursive "make install" really. Only that I need to get libmarpa to build now, and that's C-specific stuff. Yuck.

[rns]

On Tue, Jan 21, 2014 at 11:53 PM, Deyan Ginev notifications@github.comwrote:

@rns https://github.com/rns if you can join IRC, that would help me a bit. As it stands, I am missing an expected cpan/libmarpa directory in the repository, maybe I need to do some sort of recursive checkout or.. ?

Nope, cpan/libmarpa is in the repo. Just make sure you've git cloned the right branch.

—

Reply to this email directly or view it on GitHubhttps://github.com/jeffreykegler/Marpa--R2/issues/78#issuecomment-32967867 .

[dginev]

Thanks for all of the extensive IRC help @rns , I am happy to report I have built my own Marpa for the first time! Will play around with it tomorrow, however, since it is time to hibernate over here. Will let you know how things go.

[rns]

Great, thanks.

On Wed, Jan 22, 2014 at 12:32 AM, Deyan Ginev notifications@github.comwrote:

Thanks for all of the extensive IRC help @rns https://github.com/rns , I am happy to report I have built my own Marpa for the first time! Will play around with it tomorrow, however, since it is time to hibernate over here. Will let you know how things go.

— Reply to this email directly or view it on GitHubhttps://github.com/jeffreykegler/Marpa--R2/issues/78#issuecomment-32971814 .

[jeffreykegler]

Thanks much for doing this, @rns, @jddurand, @dginev.

With regard to when I work on this into the main line of development: as you may have heard Phase 2 is going very well, so in a few days there will be another inter-phase. At that point, I will carefully review this for incorporation into a development release.

I will keep this issue open until then.

[rns]

This commit shows an example of using rule_closure() in an ASF traverser.

I cherry-picked the commits making up rule_closure() to this PR for easier merging and keeping related things together.

I'm planning to convert the other 2 traversers to using rule_closure() soon.

[rns]

sl_panda1.t now uses only closures obtained from the grammar via $panda_recce->rule_closure() and rules' values got via $glade->rh_values() in the traverser code.

This whole thing (rule_closure(), rh_values(), and commented test) is now at this PR.

[jeffreykegler]

I did the merge of #93. It required a hand merge. In the process, I inadvertently reverted the fix in #89 (but nothing else I hope!). I put that back in by hand.

@rns -- could you check that the merge as it finally ended up at the head of my master branch looks right? The one file that actually reported conflicts was Value.pm.

I also undid the new tests in t/sl_null_example.t -- the test worked by comparing closure pointers numerically, which is something I avoid. (C90 forbids it and nothing in the Perl docs says it's safe, and I'm afraid I'm a bit of a purist.) Anyway, there seemed to be sufficient testing in t/sl_panda1.t.

Github closed #93 automatically when I did the merge, which I did not want until these questions were resolved --sometimes github is a little too smart. That's why I'm talking about pull request #93 here.

[rns]

On Sat, Jan 25, 2014 at 7:37 AM, Jeffrey Kegler notifications@github.comwrote:

I did the merge of #93https://github.com/jeffreykegler/Marpa--R2/pull/93. It required a hand merge. In the process, I inadvertently reverted the fix in #89 https://github.com/jeffreykegler/Marpa--R2/pull/89 (but nothing else I hope!). I put that back in by hand.

I re-checked spaces in array descriptors and they work.

Should we add a test for them? I mean they are in the test suite already, but just a special file so we knew exactly if they are broken?

I'd do it if you're ok with that.

@rns https://github.com/rns -- could you check that the merge as it finally ended up at the head of my master branch looks right? The one file that actually reported conflicts was Value.pm.

Everything's been merged great, thanks a lot. I did a couple of touchups on rule_closure() code I've written and filed a separate PR — https://github.com/jeffreykegler/Marpa--R2/pull/102

I also undid the new tests in t/sl_null_example.t -- the test worked by comparing closure pointers numerically, which is something I avoid. (C90 forbids it and nothing in the Perl docs says it's safe, and I'm afraid I'm a bit of a purist.) Anyway, there seemed to be sufficient testing in t/sl_panda1.t.

Can't agree more, in fact: I remember reading that stringifying refs is bad practice and felt uneasy about that then, but that was just a quickhack to test rule_closure() before I figured out how ASF::traverse() really works (a separate topic I'll follow up soon), so I should have done it myself (and even thought to), but have forgotten. So, thanks for correcting that. Code review is a great thing.

Github closed #93 https://github.com/jeffreykegler/Marpa--R2/pull/93automatically when I did the merge, which I did not want until these questions were resolved --sometimes github is a little too smart. That's why I'm talking about pull request #93https://github.com/jeffreykegler/Marpa--R2/pull/93here.

Ok, got it.

— Reply to this email directly or view it on GitHubhttps://github.com/jeffreykegler/Marpa--R2/issues/78#issuecomment-33281788 .

[jeffreykegler]

@rns: Re a dedicated test for spaces in descriptors. Believe it or not, I'm trying to control the number of test scripts. In t/sl_gia.t, the tests have a general template -- they allow you to specify grammar, input and expected AST, and I try to add new tests there whenever they fit the template. Would that work in this case? If so, please followup via a separate pull request.

[jeffreykegler]

It looks like this one is finished, so I'll close it. Thanks @dginev and @rns! If there's any followup, just open another issue/pull request.

[rns]

On Sat, Jan 25, 2014 at 6:57 PM, Jeffrey Kegler notifications@github.comwrote:

@rns https://github.com/rns: Re a dedicated test for spaces in descriptors. Believe it or not, I'm trying to control the number of test scripts.

Believe it or not, I do (hmm, nice recursion :) ) — or why would I be asking? :) Everything is fair if you pre-declare, as the saying goes.

In t/sl_gia.t, the tests have a general template -- they allow you to specify grammar, input and expected AST, and I try to add new tests there whenever they fit the template. Would that work in this case? If so, please followup via a separate pull request.

Ok, sl_gia_err.t looks like being able to do a better job — it specifically tests parsing of grammars with various syntax so I'll followup with a PR to it.

— Reply to this email directly or view it on GitHubhttps://github.com/jeffreykegler/Marpa--R2/issues/78#issuecomment-33293611 .