Re: incorrect yychar for unambiguous GLR

[Joel E. Denny]

On Tue, 10 Jan 2006, Paul Hilfinger wrote:

> > @@ -761,6 +761,11 @@ struct yyGLRState {
> >  
> >  struct yyGLRStateSet {
> >    yyGLRState** yystates;
> > +  /** During nondeterministic operation, yylookaheadStatuses tracks which
> > +   *  stacks have actually needed the current lookahead.  During 
> > deterministic
> > +   *  operation, yylookaheadStatuses[0] is not maintained since it would 
> > merely
> > +   *  duplicate yychar != YYEMPTY.  */
> > +  yybool* yylookaheadStatuses;
> >    size_t yysize, yycapacity;
> >  };
> >  
> 
> I am not fond of the name 'yylookaheadStatuses'; doesn't seem too
> descriptive, and one doesn't often say that "the status of X is
> true".  How about yylookaheadUnused? yylookaheadAvailable?

I agree about yylookaheadStatuses.  However, yylookaheadAvailable sounds 
like it means yychar != YYEMPTY.  The polarity of yylookaheadUnused is 
confusing, and it sounds like it might indicate whether the lookahead has 
been shifted.  How about yylookaheadNeeds, which follows from my comments 
above its declaration and maintains the plural?  I'm open to other 
suggestions of course.

Also, the documentation likes to hyphenate `look-ahead'.  Should it be 
yylookAheadNeeds (capital 'A')?  Is Paul Eggert the authority on this 
issue?

> As things stand, this organization will slightly expand the size of a
> yyGLRStackItem in some cases.  How about the following definition of 
> yySemanticOption instead?
> 
>     struct yySemanticOption {
>       /** Type tag: always false. */
>       yybool yyisState;
>       /** The lookahead symbol for this reduction.  */
>       short yyrawchar;
>       /** Semantic value of the lookahead for this reduction. */
>       YYSTYPE yyval;
>       /** Location of the lookahead for this reduction. */
>       YYLTYPE yyloc;
>       /** The last RHS state in the list of states to be reduced. */
>       yyGLRState* yystate;
>       /** Next sibling in chain of options. To facilitate merging,
>        *  options are chained in decreasing order by address. */
>       yySemanticOption* yynext;
>     };

That makes sense.  I suppose yyrule can go after yyloc.

> As long as we're adding a comment, we might as well make it complete.
> How about:
> 
>    /** Add a new semantic action that will execute the action for rule
>     *  RULENUM on the semantic values in RHS to the list of
>     *  alternative actions for STATE.  Assumes that RHS comes from 
>     *  stack #K of *STACKP. */

That's fine with me.  Since these are your words, would you like to commit 
this change?

>  > @@ -1505,9 +1540,17 @@ yysplitStack (yyGLRStack* yystackp, size
>  >                                        * sizeof yynewStates[0])))))
>  >    yyMemoryExhausted (yystackp);
>  >        yystackp->yytops.yystates = yynewStates;
>  > +      if (! (yynewLookaheadStatuses =
>  > +       (yybool*) YYREALLOC (yystackp->yytops.yylookaheadStatuses,
>  > +                            ((yystackp->yytops.yycapacity)
>  > +                             * sizeof yynewLookaheadStatuses[0]))))
>  > +  yyMemoryExhausted (yystackp);
> 
> Call me fussy, but I am not fond of this use of NULL pointers to mean 
> false.  Also, why the extra parentheses?  Could we try
> 
>       yynewLookaheadUnused =
>        (yybool*) YYREALLOC (yystackp->yytops.yylookaheadUnused,
>                             (yystackp->yytops.yycapacity
>                               * sizeof yynewLookaheadUnused[0]));
>       if (yynewLookaheadUnused == NULL)
>          yyMemoryExhausted (yystackp);
> 
> instead?

In this case, I duplicated the style of the lines immediately before this.  
I agree that it's a bit hard to read.  I'm not sure whose style that was, 
but I'd prefer to leave the rewrite up to you.

I believe gcc likes the extra parentheses when you have an assignment used 
as a boolean expression.

>  > +/*ARGSUSED*/ static void
>  >  yyreportAmbiguity (yySemanticOption* yyx0, yySemanticOption* yyx1,
> 
> This seems to be the only use of ARGSUSED in the directory.  I believe
> that the custom in Bison has been to use YYUSE instead.

Paul Eggert started using ARGSUSED together with YYUSE because, despite 
everyone's best efforts, no YYUSE definition managed to pacify all the 
lint implementations we tried without imposing limitations on bison.  
There are several occurrences of ARGSUSED in "glr.c" and "c.m4" now.

> [Aside:  Are people still using lint?  I haven't in well over a decade,
>  now that we have ANSI-style C + gcc -Wall.] 

I believe the push to satisfy lint started with a user request.  It's all 
buried somewhere in the mailing lists, but Paul Eggert probably knows the 
history best.

Thanks for reviewing my patch.

Joel