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

[Joel E. Denny]

On Fri, 24 Apr 2009, Joel E. Denny wrote:

> On Tue, 21 Apr 2009, Eric Blake wrote:
> 
> > Joel E. Denny <jdenny <at> clemson.edu> writes:

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

I've done that in the patch below.

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

I understand what the original author meant, but I think the sentences are 
redundant in the current context, so I've removed 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.

I'm using $10 and above now.

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

I no longer need $# here.

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

I no longer need either macro here.

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

I think I see your point now.  I was converting $4==[] to $4==[[]], so the 
decision of whether to treat $#==3 the same as $4==[] wasn't really 
deferred to AT_PARSER_CHECK.  I think trying to avoid $10 and above is 
what sent me down the wrong path here.

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

I'm using m4_ifval with m4_dquote now.

I pushed the following patch to branch-2.5 and master.

>From cba975069a746bccdd2f2a954954ac7e43a47ac2 Mon Sep 17 00:00:00 2001
From: Joel E. Denny <address@hidden>
Date: Wed, 22 Jul 2009 15:06:49 -0400
Subject: [PATCH] Some M4 cleanup in the testsuite.

Suggested by Eric Blake at
<http://lists.gnu.org/archive/html/bison-patches/2009-04/msg00083.html>.
* tests/existing.at (_AT_TEST_EXISTING_GRAMMAR): Do not
complicate the code by distinguishing between a missing value
and an empty string value for an optional argument.  This fix is
allowed by the similar fix in AT_TEST_TABLES_AND_PARSE below.
* tests/local.at (_AT_TEST_TABLES_AND_PARSE): Merge into...
(AT_TEST_TABLES_AND_PARSE): ... this now that the special
arguments are not needed because of the following changes.
Fix stale comments.
Bison developers should use GNU M4 and should not use
POSIXLY_CORRECT when building the test suite, so do not
complicate the code by avoiding $10 and above.
Do not quote an empty string value for an optional argument, and
do not distinguish between a missing value and an empty string
value.

diff --git a/tests/existing.at b/tests/existing.at
index b754f3c..976ab0c 100644
--- a/tests/existing.at
+++ b/tests/existing.at
@@ -45,24 +45,18 @@ AT_TEST_TABLES_AND_PARSE([$2[: LALR(1)]], [[LALR]], 
[[last-state]],
                          [[%define lr.type "LALR"
 ]$3],
                          [$4], [$5], [$6], [$7],
-                         [AT_LALR1_DIFF_CHECK([$8])$9]m4_if($#, 8, [],
-                                                            $#, 9, [],
-                                                            [, m4_shiftn(9,
-                                                                         $@)]))
+                         [AT_LALR1_DIFF_CHECK([$8])$9], [$10], [$11], [$12])
 AT_TEST_TABLES_AND_PARSE([$2[: IELR(1)]], [[IELR]], [[last-state]],
                          [[%define lr.type "IELR"
 ]$3],
                          [$4], [$5], [$6], [$7],
-                         [AT_LALR1_DIFF_CHECK([$8])$9]m4_if($#, 8, [],
-                                                            $#, 9, [],
-                                                            [, m4_shiftn(9,
-                                                                         $@)]))
+                         [AT_LALR1_DIFF_CHECK([$8])$9], [$10], [$11], [$12])
 AT_TEST_TABLES_AND_PARSE([$2[: Canonical LR(1)]], [[canonical LR]],
                          [[last-state,no-xml]],
                          [[%define lr.type "canonical LR"
 ]$3],
                          [$4], [$5], [$6], [$7],
-                         [$9]m4_if($#, 8, [], $#, 9, [], [, m4_shiftn(9, $@)]))
+                         [$9], [$10], [$11], [$12])
 
 m4_popdef([AT_LALR1_DIFF_CHECK])
 ])
diff --git a/tests/local.at b/tests/local.at
index 91e0f20..38cd295 100644
--- a/tests/local.at
+++ b/tests/local.at
@@ -405,12 +405,9 @@ m4_define([AT_PARSER_CHECK],
 #                                      [[syntax error, unexpected 'b', 
expecting $end
 #   ]])])
 m4_define([AT_TEST_TABLES_AND_PARSE],
-[_AT_TEST_TABLES_AND_PARSE($[1], address@hidden, $@)])
+[m4_pushdef([AT_COND_CASE], [m4_case([$2], $][@)])
 
-m4_define([_AT_TEST_TABLES_AND_PARSE],
-[m4_pushdef([AT_COND_CASE], [m4_case([$4], $][@)])
-
-AT_SETUP([$3])
+AT_SETUP([$1])
 
 AT_DATA_GRAMMAR([[input.y]],
 [[%code {
@@ -419,11 +416,11 @@ AT_DATA_GRAMMAR([[input.y]],
   static int yylex (void);
 }
 
-]$6[
+]$4[
 
 %%
 
-]$7[
+]$5[
 
 %%
 
@@ -437,7 +434,7 @@ static int
 yylex (void)
 {
   static int const input[] = {
-    ]m4_if([$8], [], [], [$8], [[]], [], [$8[, ]])[0
+    ]m4_if([$6], [], [], [$6], [[]], [], [$6[, ]])[0
   };
   static int const *inputp = input;
   return *inputp++;
@@ -450,40 +447,32 @@ main (void)
 }
 ]])
 
-# 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.
+# In some versions of Autoconf, 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 7 thru 12 expand to properly quoted
+# strings.
 
-# 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.
-m4_if(m4_index(m4_quote($5), [no-xml]), -1,
+m4_if(m4_index(m4_quote($3), [no-xml]), -1,
       [AT_BISON_CHECK],
       [AT_BISON_CHECK_NO_XML])([[--report=all --defines -o input.c input.y]],
-                               [0], [], m4_dquote($9))
+                               [0], [], m4_dquote($7))
 
-# Sigh.  Some M4's can't reference arg 10 directly.
-m4_pushdef([arg10], m4_car(m4_shiftn(9, $@)))
-m4_if(m4_index(m4_quote($5), [last-state]), -1,
+m4_if(m4_index(m4_quote($3), [last-state]), -1,
       [AT_CHECK([[sed -n '/^state 0$/,$p' input.output]], [[0]],
-                m4_dquote(arg10))],
+                m4_dquote($8))],
       [AT_CHECK([[sed -n 's/^state //p' input.output | tail -1]], [[0]],
-                m4_dquote(arg10)[[
+                m4_dquote($8)[[
 ]])])
-m4_popdef([arg10])
 
-m4_if($#, 10, [], m4_car(m4_shiftn(10, $@)))
+$9
 
 AT_COMPILE([[input]])
 
-m4_pushdef([AT_EXPAND_ARGS], [$][*])
-m4_pushdef([AT_DQUOTE_EACH], [[[$1]]m4_if($][#, 1, [], [, 
AT_DQUOTE_EACH(m4_shift($2))])])
-
-AT_PARSER_CHECK([[./input]]m4_if($#, 10, [], $#, 11, [], [, 
AT_DQUOTE_EACH(AT_EXPAND_ARGS(m4_shiftn(11, $@)))]))
-
-m4_popdef([AT_DQUOTE_EACH])
-m4_popdef([AT_EXPAND_ARGS])
+AT_PARSER_CHECK([[./input]],
+                m4_ifval([$10], [m4_dquote($10)]),
+                m4_ifval([$11], [m4_dquote($11)]),
+                m4_ifval([$12], [m4_dquote($12)]))
 
 AT_CLEANUP
 
-- 
1.5.4.3