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

[Joel E. Denny]

Hi Eric,

On Tue, 21 Apr 2009, Eric Blake wrote:

> Joel E. Denny <jdenny <at> clemson.edu> writes:
> 
> A review of the autotest constructs you used:

I wrote much of this code a couple of years ago.  At the time, I had 
convinced myself that my statements were true, but their validity may have 
changed, and I didn't think to check every line again.  Thanks for the 
review.

> > +# 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])
> > +# -------------------------------------------------------------
> > +
> > +# AT_CHECK invokes AS_ESCAPE before expanding macros, so it corrupts some
> > +# special characters in the macros.  To avoid this, expand now and pass it
> > +# the result with proper string quotation.  Assume args 9 thru 14 expand to
> > +# properly quoted strings.
> 
> This statement is not always true.  Ultimately, I would like to fix AS_ESCAPE 
> to expand before applying escapes.  But even without that fix, autoconf 2.63b 
> uses m4_expand so that AT_CHECK has already expanded m4 macros prior to 
> adding 
> shell escapes.

So, adding "in some versions of autoconf" would make the statement true 
now?

> > +# Pass plenty of options, to exercise plenty of code, even if we
> > +# don't actually check the output.  But SEGV is watching us, and
> > +# so might do dmalloc.
> 
> This sentence reads awkwardly.

Those sentences migrated here from another part of Bison's test suite, but 
I don't remember where, and I don't remember why I kept them.  I'll 
research it.  If I can't figure out what the original author meant, I 
suppose I'll drop them.

> > +m4_if(m4_index(m4_quote($5), [no-xml]), -1,
> > +      [AT_BISON_CHECK],
> > +      [AT_BISON_CHECK_NO_XML])([[--report=all --defines -o input.c 
> > input.y]],
> > +                               [0], [], m4_dquote($9))
> > +
> > +# Sigh.  Some M4's can't reference arg 10 directly.
> > +m4_pushdef([arg10], m4_car(m4_shiftn(9, $@)))
> 
> But autotest implies the use of GNU m4, which DOES handle arg 10 directly.  
> Well, except for when POSIXLY_CORRECT, where a future version of m4 will 
> spell 
> it ${10} to comply with POSIX instead of $10.

I don't remember what usage prompted me to do this.  Maybe it was POSIX, 
or maybe it just didn't occur to me that only Bison developers generate 
the test suite, so we probably don't need to worry about this issue.

> I guess m4sugar should add 
> m4_argn to make this easier (your use of m4_car(m4_shiftn) is not the most 
> optimal way to pick off an arbitrary argument).

I suppose I can just switch to $10.

> > +m4_if(m4_index(m4_quote($5), [last-state]), -1,
> > +      [AT_CHECK([[sed -n '/^state 0$/,$p' input.output]], [[0]],
> > +                m4_dquote(arg10))],
> > +      [AT_CHECK([[sed -n 's/^state //p' input.output | tail -1]], [[0]],
> > +                m4_dquote(arg10)[[
> > +]])])
> > +m4_popdef([arg10])
> > +
> > +m4_if($#, 10, [], m4_car(m4_shiftn(10, $@)))
> 
> Underquoting $# is asking for problems, when the # gets treated as an m4 
> comment.

I wasn't aware of this issue, but I'm still not sure I understand it.  
Can you point me to an example where this causes trouble?

> > +
> > +AT_COMPILE([[input]])
> > +
> > +m4_pushdef([AT_EXPAND_ARGS], [$][*])
> > +m4_pushdef([AT_DQUOTE_EACH], [[[$1]]m4_if($][#, 1, [], [, AT_DQUOTE_EACH
> (m4_shift($2))])])
> 
> This looks like you are trying to rewrite m4_dquote_elt.

Thanks for pointing that out.  m4_dquote_elt appears to be perfectly 
equivalent for this purpose.  What does "elt" mean?

> > +
> > +AT_PARSER_CHECK([[./input]]m4_if($#, 10, [], $#, 11, [], [,
> > AT_DQUOTE_EACH(AT_EXPAND_ARGS(m4_shiftn(11, $@)))]))
> 
> Untested, but I think this would be nicer than going through the rigamarole 
> of 
> defining AT_EXPAND_ARGS and AT_DQUOTE_EACH, and probably a bit more robust.  
> Although it may require you to bump bison's minimum autoconf requirement.  It 
> also depends on AT_PARSER_CHECK being written to treat $#==3 the same as when 
> $#==4 and $4==[], even though your current definition of 
> _AT_TEST_TABLES_AND_PARSE violates that rule of thumb (that is, it is more 
> robust to write macros that check whether an argument is empty, rather than 
> checking whether the argument was supplied).

I'm not sure how my current _AT_TEST_TABLES_AND_PARSE violates that rule 
of thumb in any significant way.  I think it just defers the decision of 
whether to follow that rule to AT_PARSER_CHECK.

> AT_PARSER_CHECK([[./input]], m4_expand([$12]), m4_expand([$13]),
>                 m4_expand([$14]))

That leaves the result of the expansion underquoted in the eventual 
AT_CHECK invocation, so it gets expanded again.  That doesn't seem to 
break the current test suite, but it's not robust.

As always, thanks for your help, Eric.