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

[Ralf Wildenhues]

* 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//

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

> > > +        exit 77
> > > +      else

By the way, the 'else' part can be taken out of the conditional,
since the 'then' part exits anyway.

> > > +        # 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, 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.  Maybe that is already sufficient to
understand the whole block of code.

Thanks,
Ralf