Re: [PATCH] {yacc-work} configure: look for a lex program to be used by the testsuite

[Stefano Lattarini]

On Saturday 29 January 2011, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Sat, Jan 29, 2011 at 12:19:27PM CET:
> > On Saturday 29 January 2011, Ralf Wildenhues wrote:
> > > You need to withstand copy and paste.  This comment is copied and pasted
> > > from a few lines above.  It is erroneous (search for 'yacc') and immoral
> > > (towards a reader of the code) to do so.  Please find a formulation and
> > > one comment that applies to both AC_CHECK_PROGS lines that can then come
> > > right after one another.
> > > 
> > > Resist copy and paste!
> > >
> > What about the following?
> > 
> >  # The test suite will have to skip some tests if no lex or yacc
> 
> s/have to//
> 
Fixed.

> >  # program is available.
> >  # We don't use AC_PROG_LEX nor AC_PROG_YACC here because:
> >  #  1. we don't want flex (resp. bison) to be preferred to system lex
> >  #     (resp. system yacc);
> >  #  2. we don't want $LEX (resp. $YACC) to be defined to ':' (resp. 'yacc')
> >  #     by default;
> >  #  3. we prefer not to have the variables YFLAGS, LEX_OUTPUT_ROOT and
> >  #     LEXLIB to be calculated and/or AC_SUBST'd;
> >  #  4. we prefer that the YACC and LEX variables are not reported in the
> >  #     configure help screen.
> >  AC_CHECK_PROGS([YACC], [yacc byacc 'bison -y'], [false])
> >  AC_CHECK_PROGS([LEX], [lex flex], [false])
> 
> That's good.  Alternatively, you could have written:
> 
>   # Similar reasoning holds for lex and flex.
> 
> 
> > > > +    lex)
> > > > +      if test x"$LEX" = x"false"; then
> > > > +        # No lex program was found at configure time, or the user has
> > > > +        # explicitly told he doesn't want a lex program to be used.
> > > > +        echo "$me: \$LEX is \"false\", skipping test" >&2
> > > 
> > > That error message could be more helpful:
> > >   $me: no working \$LEX found at configure time, or disabled
> > > 
> > > obviating the need for the comment too.
> > >
> > I agree.  But since there's also a similar pre-existing suboptimal
> > message for the 'yacc' prerequisite, couldn't I fix them both in a
> > follow-up patch instead?
> 
> Sure.  Whatever.
>
OK, I will submit that patch shortly.

> > > > +        exit 77
> > > > +      else
> 
> By the way, the 'else' part can be taken out of the conditional,
> since the 'then' part exits anyway.
>
Agreed.  Fixed.

> > > > +        # Make LEX available to configure by exporting it.
> > > 
> > > Superfluous comment (the code already shows what you do).
> > >
> > But it doesn't shows why;
> 
> It is totally clear to me what this code does and why it is there.
> I mean, all the whole $required loop does is this very same thing
> over and over again for several tools.  There are details that differ
> between the different tools, but this is not one of them.
> 
> If it is not totally clear to you,
>
No, it's perfectly clear now, it just wasn't clear the first times
I was reading `tests/defs'; at least until ...

> then by all means leave the comment in, or use this one:
> 
> >   # Make LEX available to configure.
> 
> But then I really wonder how you can understand the entries for
> gcj, g++, icc.  They all lack comments!  Oh, there's a comment
> for the gcc entry already.
>
... I read this comment.  Thus I guess the present situation is
good enough.

> Maybe that is already sufficient to understand the whole block
> of code.
> 
Agreed.  Comment removed.

Thanks,
  Stefano