Re: [PATCH] Implement %define lr.default_rules.

[Joel E. Denny]

On Tue, 21 Apr 2009, Akim Demaille wrote:

> Le 20 avr. 09 ? 07:48, Joel E. Denny a ?crit :
> 
> Hi Joel,

Hi Akim.

> > I haven't pushed yet because I would really like to rename "default rule"
> > to "default reduction" throughout Bison first:

> > Does my argument make sense to anyone else?  Is this terminology too
> > standard to change now?  If you agree with the change, should it be
> > "default reduction" or "default reduce"?  Thanks.
> 
> I agree with the change, and I would use "default reduction".

Thanks.  I'll work on a patch to make that change.

> > +  /* We need a lookahead either to distinguish different reductions
> > +     (i.e., there are two or more), or to distinguish a reduction from a
> > +     shift.  Otherwise, it is straightforward, and the state is
> > +     `consistent'.  However, for states that have any rules, treat only
> 
> I don't understand "states that have any rules".

It's been a couple of years since I wrote that, but I believe I was trying 
to be consistent with "default rules".  I agree it's ugly for many 
reasons.  I will change it to "states that have any reductions".

> > +     the accepting state as consistent (since there is never a lookahead
> > +     token that makes sense there, and so no lookahead token should be
> > +     read) if the user has otherwise disabled default rules.  */

And of course that becomes "default reductions".

> > @@ -244,6 +245,7 @@ print_reductions (FILE *out, state *s)
> >   rule *default_rule = NULL;
> >   size_t width = 0;
> >   int i, j;
> > +  bool non_default_action = false;
> 
> I'm not fond of negated Boolean variables.  Especially seems it seems used
> only negated itself :)  But then, I can understand "false" as "not known yet"
> and "true" as "known".

I'll change it to the opposite, default_reduction_only.

> > +AT_BISON_CHECK([[input.y]], [[1]], [[]],
> > +[[input.y:1.9-24: invalid value for %define variable `lr.default_rules':
> > `bogus'
> > +]])
> 
> It should be easy to report the set of valid values too.  Sort of
> unexpected/expected :)

I agree.  I'll try to work on that.

> > +# AT_TEST_TABLES_AND_PARSE(TITLE, COND-VALUE, TEST-SPEC,
> > +#                          DECLS, GRAMMAR, INPUT,
> > +#                          BISON-STDERR, TABLES-OR-LAST-STATE,
> > +#                          [OTHER-CHECKS],
> > +#                          [PARSER-EXIT-VALUE],
> > +#                          [PARSER-STDOUT], [PARSER-STDERR])
> 
> Wow, this is a m4sterpiece of 4rt.

:)  I have to admit that its usage is very specific and complex.  
However, in my complete IELR testing, I found it useful many times, and 
it's useful again in some of my other offline work.  If there's a better 
way, I'm open to it.  Also, I need to address Eric Blake's concerns.

After seeing your and Eric's helpful reviews of this patch, I feel bad 
about pushing so many major patches at once.  I didn't expect the 
immediate response, and I felt I had held on to IELR for too long already.