Re: [PATCH v3 03/15] Read CTF by the ctf target

[Tom Tromey <tromey@redhat.com>]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> +AC_ARG_WITH(babeltrace,
Yao> +	    AC_HELP_STRING([--with-babeltrace],
Yao> +			   [Specify prefix directory for the installed
Yao> +			   BABELTRACE package Equivalent to
Yao> +			   --with-babeltrace-include=PATH/include
Yao> +                           plus --with-babeltrace-lib=PATH/lib]))

I think this is missing a period and a space between "package" and
"Equivalent", but see below.

Yao> +if test "x$with_bt_include" != x; then
Yao> +  LIBBABELTRACE_CFLAGS="-I$with_bt_include "
Yao> +fi
Yao> +if test "x$with_bt_lib" != x; then
Yao> +  LIBBABELTRACE="-L$with_bt_lib -lbabeltrace -lbabeltrace-ctf"
Yao> +fi
Yao> +
Yao> +if test "x$with_babeltrace" != "xno"; then

This only checks for libbabeltrace if --with-babeltrace is specified.
So, the text above about equivalency isn't really correct, at least the
way I read it; if you want to give --with-babeltrace-include, you have
to also give --with-babeltrace.

I'm not sure how to reword the above to make it more clear.
Perhaps spelling out the options.

Yao> +
Yao> +static void
Yao> +ctf_close_dir (void)

Lots of functions missing intro comments.
Most of the comments can be really short, since these are just
implementations of target methods.

Yao> +	      /*
Yao> +	      gdb_assert (mlen == bt_ctf_get_array_len (decl));
Yao> +	      contents = bt_ctf_get_char_array (array);
Yao> +	      */

I think just delete this.

Yao> +static int
Yao> +ctf_has_all_memory (struct target_ops *ops)
Yao> +{
Yao> +  return 0;
Yao> +}

Unfortunately for you there is plenty of this patch I don't feel
competent to review.  I don't know much about the target API.

If nobody else looks after a decent interval, ping me and I will ok it.
I suppose if these methods parallel the existing tfile target, then that
is pretty good evidence for it being ok.

The rest seems ok to me.

Tom