Re: push parser

[Bob Rossi]

On Tue, Dec 05, 2006 at 08:12:01PM -0500, Joel E. Denny wrote:
> 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.

Yes, I'll try.

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

No, no one requested that name. I think it came from Odd Olsen's
original patch. I like the name you suggested, and would pretty much not
mind any consistent reasonable name.

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

Yup, that sounds good. I like yypstate, I'll change it to that.

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

Actually, currently, the user can not access the fields at all. That's
what I'm trying to say. The user can not index into the struct, because
the struct is only forward declared in the users translation unit. I
think it's good to not allow the user access to this structure. However,
if it's desired and thought wise, it can be done.

> > > 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 see. Well, they don't make sense to put together. However, the
push-parser and the pure-parser are still very different in the yacc
skeleton. Sorry if I'm explaining this, and you already know, but at
least we will be on the same page. The original yacc skeleton has
several variables in the function yyparse, and 4 variables that are
global.  The pure-parser has all the variables declared in the function
yyparse. The push-parser declares a lot of the variables in a structure, 
which is passed in and out of yyparse. My point in saying this is to say
that there is no real correlation between push-parser and pure-parser.
In fact, if the user sets both options, it should be an error.

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

OK, that makes sense. Again, I'm a bison newbie. If there is precident
for this sort of thing, I'd be happy to follow it. My opinion, with my
small knowledge of bison and it's history, is to do it the way I did
it. With that said, I'm easy to work with, and would take your
suggestions if you think it's wise.

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

I see, you'd like all occurences of yyparse to be completly removed in 
push-parser mode, if we don't add #8. Yes, that makes complete sense.

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

OK, I'm going to get the current patch reworked and resubmit it. If that
is acceptable, we should take out these issues pretty quickly.

Thanks,
Bob Rossi