Re: push parser

[Bob Rossi]

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

Thanks for your help!
Bob Rossi

On Mon, Dec 04, 2006 at 06:15:46PM -0500, Joel E. Denny wrote:
> Well, I found a little time much earlier than I thought....
> 
> I haven't been following the push parser development closely, so I decided 
> to browse this thread a little to catch up.  I'll read the latest patch in 
> detail later, but I have a few initial questions and comments:
> 
> 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?

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

> 3. How about an underscore between words for readability?  Combining #1, 
> #2, and #3:
> 
>   struct yypstate state;
>   int status;
>   YYSTYPE my_lval;
>   YYLTYPE my_lloc;
> 
>   yypstate_init (&state);
>   do {
>     status = yypush_parse (state, yylex (&my_lval, &my_lloc),
>                            &my_lval, &my_lloc);
>   } while (status == YYPUSH_MORE);
> 
> Notice that the word separation in yypush_parse is consistent with 
> %push-parser.  The only other new underscore is in yypstate_init.

This seems reasonable to me, except I don't like declaring the state
variable on the stack, for the reasons mentioned above.

> 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 could write an m4 macro that would tell me when both of these is on,
and use that, if that is acceptable to you.

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

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

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

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

Thanks,
Bob Rossi