Re: [PATCH] {master} Extend, fix and improve tests on Lex and Yacc support.

[Stefano Lattarini]

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