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

[Eric Blake]

Joel E. Denny <jdenny <at> clemson.edu> writes:

A review of the autotest constructs you used:

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

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

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

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

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

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

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

> +
> +m4_popdef([AT_DQUOTE_EACH])
> +m4_popdef([AT_EXPAND_ARGS])
> +
> +AT_CLEANUP
> +
> +m4_popdef([AT_COND_CASE])])
> 

-- 
Eric Blake