Re: [PATCH v2 2/2 gdb] Fix --enable-libctf, --disable-static, and tests sans CTF-capable toolchain

[Tom Tromey <tom@tromey.com>]

>>>>> "Nick" == Nick Alcock <nick.alcock@oracle.com> writes:

Thanks for doing this.  This looks basically ok, just a few nits below.

The patch should have some intro text.  gdb style is to use the same
text for the description in the email and for the commit.

Nick> +if test x${enable_static} = xno; then
Nick> +  LIBCTF="-Wl,--rpath,../libctf/.libs ../libctf/.libs/libctf.so"

Indentation of 2 here...

Nick> +if test "${enable_libctf}" = yes; then
Nick> +    AC_DEFINE(ENABLE_LIBCTF, 1, [Handle .ctf type-info sections])

... but 4 here.  I think they could be the same, whatever is most common
in the file, or most common nearby anyhow.

Nick> +#else
Nick> +
Nick> +void
Nick> +_initialize_ctfread (void)

gdb just uses "()", not "(void)" now.

Nick> diff --git a/gdb/testsuite/configure.ac b/gdb/testsuite/configure.ac
... 
Nick> +AC_ARG_ENABLE(libctf,
Nick> +[AS_HELP_STRING([--enable-libctf], [Handle .ctf type-info sections])], [

This seems to be identical to what's in gdb.
I don't really mind, but if you're duplicating this across several
subdirectories, it may be better to stick a new macro into a new .m4 in
config/, then use that everywhere.

Nick> +if {[skip_ctf_tests]} {
Nick> +    return
Nick> +}

I think these spots should call "unsupported" to explain what happened.

Nick> +proc skip_ctf_tests {} {

I suspect you probably want to use gdb_caching_proc here.  This can be
friendlier when running a lot of parallel tests, because it caches the
result, and this proc is doing a compilation.

Tom