Re: [PATCH] Support official CLooG.org versions.

[Paolo Bonzini <bonzini at gnu dot org>]

First of all, I must say I think the code is very clear, so with the 
namespacing problems Ralf pointed out cloog/cloog.m4 is ready.

I have one question:

> +# CLOOG_INIT_FLAGS ()
> +# -------------------------
> +# Provide configure switches for CLooG support.
> +# Initialize clooglibs/clooginc according to the user input.
> +AC_DEFUN([CLOOG_INIT_FLAGS],
> +[
> +  AC_ARG_WITH(cloog,
> +    [AS_HELP_STRING(
> +      [--with-cloog=PATH],
> +      [Specify prefix directory for the installed CLooG-PPL package.
> +       Equivalent to --with-cloog-include=PATH/include
> +       plus --with-cloog-lib=PATH/lib])])
> +  AC_ARG_WITH(cloog_include,
> +    [AS_HELP_STRING(
> +      [--with-cloog-include=PATH],
> +      [Specify directory for installed CLooG include files])])
> +  AC_ARG_WITH(cloog_lib,
> +    [AS_HELP_STRING(
> +      [--with-cloog-lib=PATH],
> +      [Specify the directory for the installed CLooG library])])
> +
> +  AC_ARG_ENABLE(cloog-version-check,
> +    [AS_HELP_STRING(
> +      [--disable-cloog-version-check],
> +      [disable check for CLooG version])],
> +    ENABLE_CLOOG_CHECK=$enableval,
> +    ENABLE_CLOOG_CHECK=yes)
> +
> +  # Initialize clooglibs and clooginc.
> +  case $with_cloog in
> +    no)
> +      clooglibs=
> +      clooginc=
> +      ;;
> +    "" | yes)
> +      ;;
> +    *)
> +      clooglibs="-L$with_cloog/lib"
> +      clooginc="-I$with_cloog/include"
> +      ;;
> +  esac
> +  if test "x${with_cloog_include}" != x ; then
> +    clooginc="-I$with_cloog_include"
> +  fi
> +  if test "x${with_cloog_lib}" != x; then
> +    clooglibs="-L$with_cloog_lib"
> +  fi
> +  if test "x${with_cloog}" != x && test "x${with_cloog_include}" != x \
> +     && test "x${with_cloog_lib}" != x && test -d ${srcdir}/cloog; then
> +    clooglibs='-L$$r/$(HOST_SUBDIR)/cloog/'"$lt_cv_objdir"' '
> +    clooginc='-I$$r/$(HOST_SUBDIR)/cloog/include -I$$s/cloog/include '
> +    enable_cloog_version_check=no
> +  fi
> +]
> +)

It seems to me that the "!=" in the last "if" should be "=".  Also, I 
don't think disabling the version check for an in-tree CLOOG is 
necessary, or even good.

However, I'm not sure about the placement of the code, for two reasons:

1) while we have been sloppy on this, the toplevel has no business 
determining whether a package is the correct version (historically, this 
started with the GMP/MPFR checks which were needed for Fortran but 
should now be moved away as well.  libelf checks will also disappear 
once Ian's objfile library is committed.

2) the last "if" is the only part of cloog/cloog.m4 that relies on the 
structure of the GCC build system.

So, it seems to me that the toplevel configure.ac should be basically 
oblivious of the exact mechanics of detecting CLOOG.  The good news is 
that it should be easy to implement what I propose, thanks to the very 
good modularization you have in cloog/cloog.m4.

In particular it seems to me that if you remove the last if from 
CLOOG_INIT_FLAGS, all you need in the toplevel configure.ac is

CLOOG_INIT_FLAGS
if test "x${with_cloog}" = x && test "x${with_cloog_include}" = x \
   && test "x${with_cloog_lib}" = x && test -d ${srcdir}/cloog; then
   clooglibs='-L$$r/$(HOST_SUBDIR)/cloog/'"$lt_cv_objdir"' '
   clooginc='-I$$r/$(HOST_SUBDIR)/cloog/include -I$$s/cloog/include '
fi

(I changed != to = above) and the rest of the detection can go in 
gcc/configure.ac, which is where it is really needed.  The code in 
gcc/configure.ac would be exactly what you're adding to the toplevel 
configure script.

Sorry for not noticing this before.  This kind of "big picture" problems 
are things I typically overlook when the code is bad, so you can take 
this as a compliment. :)

Paolo