Re: push parser

[Joel E. Denny]

On Tue, 5 Dec 2006, Bob Rossi wrote:

> Joel, thanks for taking the time to look at this. There is a lot of
> information below, and I hope I did justice with my response in the
> little time I have. 

Your reply was fine, but don't feel rushed.  I really should be doing 
other work anyway.

> The easiest way for me to proceed, is to get the
> current patch into bison, and then nail these issues down 1 by 1, with
> small patches. If that is OK with you, that would be best for me.

It's fine but I'm getting a lot of fuzz trying to apply the patch because 
there have been many commits since you wrote it.  Would you please post an 
updated copy?  Thanks.

> Thanks for your help!

Thanks for continuing to work on this feature, which I think a lot of 
people will enjoy.

> > 1. How was the name yypvars chosen?  What does ctx stand for?  Context?  
> > It seems we have about three terms for the same thing: parser state, vars, 
> > and context.  This will make the documentation a little confusing I fear.  
> > I think we should pick one term and stick with it.  I prefer parser state, 
> > which implies renaming yypvars to something like yypstate.
> 
> That's acceptable to me. Does anyone else need to way in on this, or
> should I submit a patch with the new names?

I guess there are a few questions that should be answered first.  Did Paul 
or Akim specifically request the name yypvars?  Is there some precedent 
for that name?  Which name do you personally like better and why?  Or are 
you indifferent?

My guess is that Paul and Akim do not have time to give their opinions on 
this.  If you and I can agree on a name, I think we should push forward 
with it.  It should be trivial to revert if there's an objection before 
the next release.

> > 2. I find it strange that yypvarsinit invokes malloc.  That would be 
> > something more like yypvarsnew, in my opinion.  Also, the user may wish to 
> > use automatic storage.  I think the following makes more sense:
> > 
> >   struct yypvars ctx;
> >   yypvarsinit (&ctx);
> 
> I have no problem changing the name. However, using automatic storage
> was intentionally disabled. Simply because it was desirable to keep the
> 'struct yypvars' structure opaque. I thought it would be better if the
> user could not get access to this data structure, since they have no
> business modifing it at this point. Also, if the user does not have
> access to this structure, it'll be easier to change the internal
> representation, without breaking the users code.

The user can access the internal fields either way.  It's just a question 
of "ctx." or "ctx->".

> > 4. If %push-parser implies %pure-parser, why not code that logic into 
> > parse-gram.y?  That is:
> > 
> >   | "%pure-parser"      { pure_parser = true; }
> >   | "%push-parser"      { push_parser = true; pure_parser = true; }
> > 
> > It appears as if you often have to check both b4_push_if and b4_pure_if 
> > when you should really only need to check b4_pure_if.  This should help.  
> > (In a similar manner, %glr-parser implies %nondeterministic-parser.  In 
> > that case, it's the front-end that's simplified rather than the back-end.)
> 
> I also am hesitant to do this, but if you really want to, I will. I
> think it's reasonable for bison to be able to ask if push-parser is on,
> and pure-parser is off. I don't understand why we would want it to think
> the user selected the pure-parser, when they didn't.

I may have missed some posts on this topic, but my understanding was that 
declaring %pure-parser is meaningless when %push-parser is declared 
because %push-parser implies %pure-parser.  That is, this:

  %push-parser

is exactly the same as this:

  %push-parser
  %pure-parser

I'm recommending we hard-code this logic in one place rather than multiple 
places.

> I could write an m4 macro that would tell me when both of these is on,
> and use that, if that is acceptable to you.

Before we discuss that possibility, we need to agree on the relationship 
between these directives.

> > 5. For yyerror arguments, why not just stick with whatever pure parsers 
> > normally use?  #4 should help.
> 
> Sorry, I'm not sure what you are talking about here. Could you explain
> in more detail?

Sorry, I was responding to your question about yyerror here:

  http://lists.gnu.org/archive/html/bison-patches/2006-10/msg00058.html

If you follow my idea in #4 above, I see no reason to alter 
b4_yyerror_args.

> > 6. I read somewhere in this thread that yypushparse will never modify its 
> > yynchar, yynlval, and yynlloc arguments.  Shouldn't they be const then?
> 
> Well, this is getting complicated. The patch that needs to be reviewed,
> and should be applied gives yypushparse these arguments. As of know,
> these arguments don't exist. It would be useful to apply this patch
> first, and then have the discussion about this.

It's fine to wait.

> > 7. Shouldn't you remove the yyparse prototype you're currently generating 
> > in push mode?
> 
> The patch I'd like removed completly removes the yyparse function in
> push mode. Are you asking about something else?

No, it leaves the prototype and a #define.  Search the patched push.c for 
yyparse.  The first three occurrences need to be addressed.

> > 8. Akim wanted there to be a yyparse for pull mode.  You and Paul have 
> > noticed that this would create a dependence on yylex even when the user 
> > isn't interested in pull mode.  Wouldn't making yyparse a C macro solve 
> > this?
> > 
> >   #define yyparse() yypull_parse (&yylex)
> > 
> > Then define yypull_parse as the function wrapping yypush_parse.  This way, 
> > yylex is never referenced unless the user invokes yyparse.
> > 
> > And if we're going to do that, I'd like for yypull_parse to accept a 
> > yypvars (or yypstate).  Then the user can combine yypush_parse and 
> > yypull_parse.  For example, the user can yypush_parse a token to select a 
> > sub-grammar and then yypull_parse the input stream.
> > 
> > Passing NULL for the yypvars could instruct yypull_parse to construct a 
> > yypvars internally, so then we'd have:
> > 
> >   #define yyparse() yypull_parse (NULL, &yylex)
> > 
> > This maintains the usual prototype of yyparse as Akim wanted.
> 
> I would be willing to do this sort of work. I'd very much like to get
> the current patches into bison, and then we can build on top of that. I
> have no objections to the idea above, and I think it's a pretty cool
> idea.

Great.