Re: proposed patch to support Unified Parallel C (version 3 of patch)

[Alexandre Duret-Lutz]

Hi Jonathan,

Congratulations on the patch, and thanks for making it complete.
It looks great, but I can see a couple of minor problems.  My
main complaint is that your tests do not show that building a
upc program actually work.

[...]

 JH> @itemx maude_LFLAGS
 JH> @itemx maude_OBJCFLAGS
 JH> @itemx maude_RFLAGS
 JH> address@hidden maude_UPCFLAGS
 JH> @itemx maude_YFLAGS

There is at least one other place in the manual that lists
*FLAGS variables and could use an update : "Flag Variables
Ordering" section of the FAQ.

[...]

 JH> address@hidden Unified Parallel C Support
 JH> address@hidden Unified Parallel C Support
 JH> +
 JH> address@hidden Unified Parallel C support
 JH> address@hidden Support for Unified Parallel C
 JH> +
 JH> +Automake includes some support for Unified Parallel C.
 JH> +
 JH> +Any package including Unified Parallel C code must define the output 
variable
 JH> address@hidden in @file{configure.ac}; the simplest way to do this is to 
use
 JH> +the @code{AM_PROG_UPC} macro (@pxref{Particular Programs, , Particular
 JH> +Program Checks, autoconf, The Autoconf Manual}).

This reference to the Autoconf manual looks wrong, since you're submitting
AM_PROG_UPC to Automake and documenting it elsewhere in this manual.

[...]

 JH> @@ -4967,6 +5010,9 @@
 JH> @vindex OBJCLINK
 JH> Objective C (@code{OBJCLINK})
 JH> @item
 JH> address@hidden UPCLINK
 JH> +Unified Parallel C (@code{UPCLINK})
 JH> address@hidden
 JH> @vindex LINK
 JH> C (@code{LINK})
 JH> @end enumerate

This part explains how the linker is chosen when languages are mixed.
I could not find the part of the patch that implements this selection, 
nor any test-case for it.

(The linker is chosen by resolve_linker() in automake.in.)

[...]

 JH> Index: m4/depend.m4
 JH> ===================================================================
 JH> RCS file: /cvs/automake/automake/m4/depend.m4,v
 JH> retrieving revision 1.37
 JH> diff -u -r1.37 depend.m4
 JH> --- m4/depend.m4   6 Jun 2006 20:55:44 -0000       1.37
 JH> +++ m4/depend.m4   31 Jul 2006 00:28:25 -0000
 JH> @@ -34,6 +34,7 @@
 JH> ifelse([$1], CC,   [depcc="$CC"   am_compiler_list=],
 JH> [$1], CXX,  [depcc="$CXX"  am_compiler_list=],
 JH> [$1], OBJC, [depcc="$OBJC" am_compiler_list='gcc3 gcc'],
 JH> +       [$1], UPC,  [depcc="$UPC"  am_compiler_list='gcc3 gcc'],
 JH> [$1], GCJ,  [depcc="$GCJ"  am_compiler_list='gcc3 gcc'],
 JH> [depcc="$$1"   am_compiler_list=])

Why restrict the dependency methods to gcc3 and gcc ?

I don't know UPC.  My understanding from
http://www.gwu.edu/~upc/download.html is that several compilers
exist, and they are not all based on gcc.  Have you used any
other ?  If The hp version is based on the hp C compiler, it
probably requires one of the "hp*" dependency methods.  I
believe it wouldn't be wrong to try them all like we do for
C/C++.
 
[...]

JH> Index: tests/upc.test
 JH> ===================================================================
 JH> RCS file: tests/upc.test
 JH> diff -N tests/upc.test
 JH> --- /dev/null      1 Jan 1970 00:00:00 -0000
 JH> +++ tests/upc.test 31 Jul 2006 00:28:26 -0000
[...]
 JH> +# Test that `.upc' extension works.
 JH> +# From Ralf Corsepius (for C++).
 JH> +
 JH> +. ./defs || exit 1
 JH> +
 JH> +cat >> configure.in << 'END'
 JH> +AM_PROG_UPC
 JH> +END
 JH> +
 JH> +cat > Makefile.am << 'END'
 JH> +bin_PROGRAMS = hello
 JH> +hello_SOURCES = hello.upc
 JH> +END
 JH> +
 JH> +$ACLOCAL || exit 1
 JH> +$AUTOMAKE || exit 1
 JH> +
 JH> +grep '^\.SUFFIXES:.*\.upc' Makefile.in

Please test that compiling a UPC "Hello World" project actually
works, by going as far as running "make" and maybe running the
generated binary itself (if that makes sense).  I'd even run
"make distcheck" just to be sure the clean rules and the like
work OK.

grepping Makefile.in is not enough to ensure that configure and
Makefile can actually build anything.  grep tests are weak.
There are too much of them in the testsuite and we should try to
move away from them.  Actually running ./configure && $MAKE to make 
sure it did what they should gives better coverage.

You really should regard the test case as a way to protect your
new feature from future changes to Automake, so use it like
you'd do in a project, demonstrating how you need the feature to
work.

As said above, another point you've documented but not
demonstrated is how upc mixes with other languages.

Regards,
-- 
Alexandre Duret-Lutz

Shared books are happy books.     http://www.bookcrossing.com/friend/gadl