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
