On Thursday 16 December 2010, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Mon, Dec 13, 2010 at 07:54:05PM CET:
> > OK to apply to a temporary branch off of maint, and merge to master?
>
> The patch is ok with nits addressed.
>
I've addressed almost all of them, but I have a doubt about one (see
defs.in below) and I disagree with one (see lex3.test below).
> > BTW, notice that I'm planning to further extend the Lex/Yacc tests
> > and make them more "semantic", but that should be better done in a
> > follow-up patch IMVHO.
>
> If there is any way I can get you to not do any more of such
> conversions: Please don't work even more on *these* tests. They are
> Good Enough[tm].
>
IMHO they're not. See just below.
> The incremental gain from more change is not worth the additional
> work from you nor review from me.
>
> Actually, lex and yacc support has *several* *real* issues, with maybe
> more than a dozen reported bugs in the last few years, many of them
> unfixed. See the list archives.
>
Yes, but I woldn't dare trying to modify the Lex/Yacc related code with
the limited understanding I have of it, without having a *much* stronger
testsuite than it's currently available.
> It would be really nice if somebody who knows their ways around
> bison/yacc and flex/lex well
>
I'm not that somebody, unfortunately.
> (or is willing to learn)
>
Well, maybe I might *try* to be this somebody... once I feel
backed up by a strong-enough testsuite.
> could look into some of the issues. I should warn that it's not
> easy though to come up with coherent/consistent and portable
> improvements.
>
> > Subject: [PATCH] Extend, fix and improve tests on Lex and Yacc support.
> >
> > * tests/lexcpp.test: New test script, on support for Lex + C++.
> > * tests/lexvpath.test: New test script, test build and rebuild
> > rules for lexers in VPATH setup.
> > * tests/yacc-basic.test: New test script, run simple "semantic"
> > checks on basic Yacc support (similarly to what lex3.test does
> > for Lex support).
> > * tests/lex.test: Don't create useless dummy source file joe.l.
> > Remove extra blank lines.
> > (Makefile.am): Remove useless definition of $(LDADD).
> > * tests/lex4.test: Add trailing `:' command. Do not create dummy
> > useless lex source file.
> > * tests/lex2.test: Likewise. Call automake with the `-a' option,
> > so that it doesn't fail for the absence of `ylwrap' script. Make
> > grepping of automake stderr stricter.
> > * tests/yacc7.test: Add trailing `:' command. Enable `errexit'
> > shell flag earlier (just after having sourced ./defs).
> > * tests/yacc4.test: Likewise. Also ...
> > (configure.in): Use pre-populated skeleton set up by ./defs,
> > instead of writing one from scratch.
> > Other minor cosmetic changes.
> > * tests/yacc5.test: Likewise.
> > * tests/yaccvpath.test: Likewise. Also ...
> > ($distdir): New variable.
> > Use it throughout.
> > * tests/lex5.test: Likewise.
> > * tests/lex3.test: Likewise. Check the distdir, rather than
> > grepping the distribution tarball. Extend the test on the
> > created binary, and be sure to avoid hangs. Add some comments.
> > * tests/yacc.test: Use stricter grepping. Add trailing `:'.
> > * tests/yacc6.test: Likewise.
> > * tests/yacc3.test: Likewise. Prefer `cp -f' over plain `cp'.
> > Do not create unused file `Makefile.sed'. Remove useless rules
> > from Makefile.am. Other minor cosmetic changes.
> > * tests/yacc2.test: Make grepping of generated `Makefile.in' and
> > of automake error messages stricter. Do not redirect output of
> > grep to /dev/null. Prefer `cp -f' over plain `cp'. Move call to
> > aclocal earlier. Reduce the number of empty blank lines. Fix a
> > typo in comments.
> > * tests/yacc8.test: Fixed bugs that reduced the completeness of
> > the tests. Added trailing `:' command.
> > (configure.in): Use pre-populated skeleton set up by ./defs,
> > instead of writing one from scratch.
> > * tests/yaccpp.test: Test also extensions `.y++', `.ypp', and
> > `.yxx', rather than only `.yy'.
> > * tests/defs.in ($required): Better recognition of requirement
> > "flex".
> > * tests/Makefile.am (TESTS): Update.
> > --- a/tests/defs.in
> > +++ b/tests/defs.in
> > @@ -113,6 +113,13 @@ do
> > echo "$me: running etags --version -o /dev/null"
> > ( etags --version -o /dev/null ) || exit 77
> > ;;
> > + flex)
> > + # Since flex is required, we pick LEX for ./configure.
> > + LEX=flex
> > + export LEX
> > + echo "$me: running flex --version"
> > + ( flex --version ) || exit 77
> > + ;;
>
> I don't understand why the new 'required=flex' entry should be needed in
> defs.in. Any tests that fail without the defs.in change?
>
Not for me, but since we do something similar for bison, I was just trying
to be consistent. Should I remove this hunk?
> > +++ b/tests/lex.test
> > @@ -26,22 +26,16 @@ END
> > cat > Makefile.am << 'END'
> > bin_PROGRAMS = zot
> > zot_SOURCES = joe.l
> > -LDADD = @LEXLIB@
>
> Please don't remove that. It is documented to be required.
>
OK (even if automake doesn't currently require it at automake-time).
> > END
>
>
> > --- a/tests/lex3.test
> > +++ b/tests/lex3.test
>
> > +# Basic semantic checks on Lex support.
> > # Test associated with PR 19.
> > # From Matthew D. Langston.
>
> > cat > Makefile.am << 'END'
> > @@ -46,38 +44,44 @@ END
> >
> > cat > foo.l << 'END'
> > %%
> > -"END" return EOF;
> > +"GOOD" return EOF;
> > .
> > %%
> > int
> > main ()
> > {
> > - while (yylex () != EOF)
> > - ;
> > -
> > - return 0;
> > + if (yylex () == EOF)
> > + return 0;
> > + else
> > + return 1;
>
> That's not "semantic" either. One wouldn't write a lexer like that.
>
Yes, but this will have IMVHO a lower risk of hanging due to possible
errors/blunders in other parts of the testcase. Hanging which, BTW,
would happen ...
> > }
> > END
>
> > +# Program should build and run.
> > $MAKE
> > -echo 'This is the END' | ./foo
> > -$MAKE distcheck
> > +echo GOOD | ./foo
> > +echo BAD | ./foo && Exit 1
>
... here.
I think we should keep the lexer my stupid but safer way.
> > --- a/tests/lex5.test
> > +++ b/tests/lex5.test
>
> > @@ -33,10 +29,9 @@ AC_OUTPUT
> > END
> >
> > cat > Makefile.am << 'END'
> > -AUTOMAKE_OPTIONS = subdir-objects
> > -LDADD = @LEXLIB@
> > -
> > -bin_PROGRAMS = foo/foo
> > +AUTOMAKE_OPTIONS = subdir-objects
> > +LDADD = @LEXLIB@
> > +bin_PROGRAMS = foo/foo
> > foo_foo_SOURCES = foo/foo.l
>
> Please. Such changes cost you time, me time, clutter up the history,
> and gain nobody anything. We've been there before.
>
Reverted (even if it's an eyesore every time I look at it).
> > END
> >
> > --- /dev/null
> > +++ b/tests/lexvpath.test
> > @@ -0,0 +1,117 @@
>
> > +# This test checks that dependent files are updated before including
> > +# in the distribution. `lexer.c' depends on `lexer.l'. The later is
>
> s/\. /. / (several instances)
> latter
>
Fixed (the spelling checker didn't help here, unfortunately).
> > +# updated so that `lexer.c' should be rebuild. Then we are running
>
> rebuilt
>
Ditto.
> > +# `make' and `make distdir' and check whether the version of `lexer.c'
> > +# to be distributed is up to date.
> > +
> > +# Please keep this in sync with sister test `yaccvapth.test'.
> > +
> > +required='gcc flex'
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +distdir=$me-1.0
> > +
[CUT]
> > +#
> > +# Now check to make sure that `make dist' will rebuild the parser.
> > +#
> > +
> > +# A delay is needed to make sure that the new parse.y is indeed newer
> > +# than parse.c, i.e. the they don't have the same timestamp.
> > +$sleep
>
> The comment was already too verbose before you copy and pasted it. ;-)
>
Indeed. I went for "Ensure that lexer.l is newer than lexer.c".
> > --- /dev/null
> > +++ b/tests/yacc-basic.test
> > @@ -0,0 +1,78 @@
>
> > +# Basic semantic checks on Yacc support.
> > +
> > +required=bison
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +distdir=$me-1.0
> > +
> > +cat >> configure.in << 'END'
> > +AC_PROG_CC
> > +AC_PROG_YACC
> > +AC_OUTPUT
> > +END
> > +
> > +cat > Makefile.am << 'END'
> > +bin_PROGRAMS = foo
> > +foo_SOURCES = parse.y foo.c
> > +END
> > +
> > +cat > parse.y << 'END'
> > +%{
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +int yylex () { return (getchar()); }
>
> space before open paren (after getchar).
>
Fixed.
> > +void yyerror (char *s) {}
> > +%}
> > +%%
> > +a : 'a' { exit(0); };
> > +END
> > +
> > +cat > foo.c << 'END'
> > +int main () { yyparse(); return 1; }
>
> see above
>
Fixed.
> > --- a/tests/yacc2.test
> > +++ b/tests/yacc2.test
> > @@ -1,5 +1,6 @@
> > #! /bin/sh
> > -# Copyright (C) 1999, 2001, 2002, 2003, 2006 Free Software Foundation,
> > Inc.
> > +# Copyright (C) 1999, 2001, 2002, 2003, 2006, 2010 Free Software
> > +# Foundation, Inc.
> > #
> > # This program is free software; you can redistribute it and/or modify
> > # it under the terms of the GNU General Public License as published by
> > @@ -26,61 +27,48 @@ AC_PROG_CC
> > AC_PROG_YACC
> > END
> >
> > +# Run it here once and for all, since we are not going to modify
> > +# configure.in anymore.
> > +$ACLOCAL
> > +
> > cat > Makefile.am <<'END'
> > bin_PROGRAMS = zardoz
> > zardoz_SOURCES = zardoz.y
> > END
> >
> > # Don't redefine several times the same variable.
> > -cp Makefile.am Makefile.src
> > +cp -f Makefile.am Makefile.src
>
> Why should you need this change? Generally, I don't see why you ever
> need 'cp -f'.
>
I dimly remember that I used to think there were cp implementations which
might prompt if stdout/stderr is a tty and the dest file exists, unless
the `-f' option is used.. But I might be very mistaken here, and since
you tell me the `-f' is useless ...
> Please undo. (several instances)
>
... I'll revert these changes.
>
> Thanks,
> Ralf
>
I'll wait replies to my question about defs.in and my objection about
lex3.test before pushing the amended patch.
Regards,
Stefano
