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

[Ralf Wildenhues <Ralf dot Wildenhues at gmx dot de>]

Hello Andreas,

a couple of nits:

* Andreas Simbürger wrote on Sat, Oct 30, 2010 at 07:43:33PM CEST:
> --- /dev/null
> +++ b/config/cloog.m4
[...]
> +AC_DEFUN([CLOOG_FIND_FLAGS],
> +[
> +  AC_REQUIRE([CLOOG_INIT_FLAGS])
> +
> +  _saved_cflags=$CFLAGS
> +  _saved_cppflags=$CPPFLAGS
> +  _saved_ldflags=$LDFLAGS
> +  _saved_libs=$LIBS

The leading underscores here ...

> +  _clooglegacyinc="-DCLOOG_PPL_BACKEND"
> +  _cloogorginc="-DCLOOG_INT_GMP -DCLOOG_ORG"
> + 
> +  dnl clooglibs & clooginc may have been initialized by CLOOG_INIT_FLAGS.
> +  CFLAGS="${CFLAGS} ${clooginc} ${pplinc} ${gmpinc}"
> +  CPPFLAGS="${CPPFLAGS} ${_clooglegacyinc} ${_cloogorginc}"
> +  LDFLAGS="${LDFLAGS} ${clooglibs}"
> +
> +  AC_CACHE_CHECK([for installed CLooG],
> +                 [gcc_cv_cloog_type],
> +    [LIBS="-lcloog ${saved_libs}"

... are not reflected here,

> +     AC_LINK_IFELSE([_CLOOG_PPL_LEGACY_PROG],
> +      [gcc_cv_cloog_type="PPL Legacy"],
> +      [LIBS="-lcloog-isl -lisl ${saved_libs}"

here,

> +       AC_LINK_IFELSE([_CLOOG_ORG_PROG],
> +        [gcc_cv_cloog_type=ISL],
> +        [LIBS="-lcloog-ppl ${saved_libs}"

and here.  The _saved_* variables names are not in a namespace specific
to your macros (_cloog_saved_libs or so); strictly that is problematic
only when the variable names are used in enclosed or enclosing macros
or configure.ac.

> +         AC_LINK_IFELSE([_CLOOG_ORG_PROG],
> +          [gcc_cv_cloog_type=PPL],
> +          [gcc_cv_cloog_type=no])])])])
> +
> +  case $gcc_cv_cloog_type in
> +    "PPL Legacy")
> +      clooginc="${clooginc} ${_clooglegacyinc}"
> +      clooglibs="${clooglibs} -lcloog"
> +      cloog_org=no
> +      ;;
> +    "ISL")
> +      clooginc="${clooginc} ${_cloogorginc}"
> +      clooglibs="${clooglibs} -lcloog-isl"
> +      cloog_org=yes
> +      ;;
> +    "PPL")
> +      clooginc="${clooginc} ${_cloogorginc}"
> +      clooglibs="${clooglibs} -lcloog-ppl"
> +      cloog_org=yes
> +      ;;
> +    *)
> +      clooglibs=
> +      clooginc=
> +      cloog_org=
> +      ;;
> +  esac
> +
> +  LIBS=$_saved_libs
> +  CFLAGS=$_saved_cflags
> +  CPPFLAGS=$_saved_cppflags
> +  LDFLAGS=$_saved_ldflags
> +]
> +)
> +
> +# _CLOOG_CHECK_CT_PROG(MAJOR, MINOR, REVISION)
> +# --------------------------------------------
> +# Helper for verifying CLooG's compile time version.
> +m4_define([_CLOOG_CHECK_CT_PROG],[AC_LANG_PROGRAM(
> +  [#include "cloog/cloog.h"],
> +  [#if CLOOG_VERSION_MAJOR != $1 \
> +    || CLOOG_VERSION_MINOR != $2 \
> +    || CLOOG_VERSION_REVISION < $3
> +    choke me
> +   #endif])])
> +
> +# _CLOOG_CHECK_RT_PROG ()
> +# -----------------------
> +# Helper for verifying that CLooG's compile time version
> +# matches the run time version.
> +m4_define([_CLOOG_CHECK_RT_PROG],[AC_LANG_PROGRAM(
> +  [#include "cloog/cloog.h"],
> +  [if ((cloog_version_major () != CLOOG_VERSION_MAJOR)
> +    && (cloog_version_minor () != CLOOG_VERSION_MINOR)
> +    && (cloog_version_revision () != CLOOG_VERSION_REVISION))
> +    {
> +      return 1;
> +    }])])
> +
> +# CLOOG_CHECK_VERSION CLOOG_CHECK_VERSION (MAJOR, MINOR, REVISION)
> +# ----------------------------------------------------------------
> +# Test the found CLooG to be exact of version MAJOR.MINOR and at least
> +# REVISION.
> +# If we're using the old CLooG-PPL (Legacy), the old version check will
> +# be executed (Ignores the provided version information).
> +AC_DEFUN([CLOOG_CHECK_VERSION],
> +[
> +  AC_REQUIRE([CLOOG_FIND_FLAGS])
> +
> +  _saved_CFLAGS=$CFLAGS
> +  _saved_LDFLAGS=$LDFLAGS
> +
> +  CFLAGS="${saved_cflags} ${clooginc} ${pplinc} ${gmpinc}"
> +  LDFLAGS="${saved_ldflags} ${clooglibs}"
> +
> +  if test "${cloog_org}" = yes ; then
> +    AC_CACHE_CHECK([for verison $1.$2.$3 of CLooG],
> +      [gcc_cv_cloog_ct_0_14_0],
> +      [AC_COMPILE_IFELSE([_CLOOG_CHECK_CT_PROG($1,$2,$3)],
> +        [gcc_cv_cloog_ct_0_14_0=yes],
> +        [gcc_cv_cloog_ct_0_14_0=no])])
> +  elif test "${cloog_org}" = no ; then
> +    AC_CACHE_CHECK([for version 0.15.5 (or later revision) of CLooG],
> +      [gcc_cv_cloog_ct_0_15_5],
> +      [AC_COMPILE_IFELSE([_CLOOG_CHECK_CT_PROG(0,15,5)],
> +        [AC_COMPILE_IFELSE([_CLOOG_CHECK_CT_PROG(0,15,9)],
> +         [gcc_cv_cloog_ct_0_15_5=yes],
> +          [gcc_cv_cloog_ct_0_15_5=buggy but acceptable])],

This will temporarily assign 'buggy' to the variable, while trying to
execute the program 'but' with argument 'acceptable'; use quotes to
avoid this.

While testing build patches, it is fairly important to try to expose all
of the different code cases, or ask somebody else to cover the cases
that you cannot expose on your system, so that such issues are caught.

> +        [gcc_cv_cloog_ct_0_15_5=no])])
> +  fi
> +
> +  CFLAGS=$_saved_CFLAGS
> +  LDFLAGS=$_saved_LDFLAGS
> +]
> +)
> +
> +# CLOOG_IF_FAILED (ACTION-IF-FAILED)
> +# ----------------------------------
> +# Executes ACTION-IF-FAILED, if GRAPHITE was requested and
> +# the checks failed.
> +AC_DEFUN([CLOOG_IF_FAILED],
> +[
> +  CLOOG_REQUESTED([graphite_requested=yes], [graphite_requested=no])
> +  
> +  if test "${gcc_cv_cloog_ct_0_14_0}" = no \
> +    || test "${gcc_cv_cloog_rt_0_14_0}" = no \
> +    || test "${gcc_cv_cloog_ct_0_15_5}" = no; then
> +    clooglibs=
> +    clooginc=
> +  fi
> +
> +  if test "${graphite_requested}" = yes \
> +    && test "x${clooglibs}" = x \
> +    && test "x${clooginc}" = x ; then
> +    $1
> +  fi
> +]
> +)

Thanks,
Ralf