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

[Doug Evans <dje@google.com>]

On Wed, Mar 13, 2013 at 1:08 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "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.

Is there a compelling need for with-babeltrace-{include,lib}?
We don't have with-expat-{include,lib} for example.
I think we should punt on them for now if there's no real need for them.

> 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 nice thing here is that I don't mind of the implementation isn't
100% perfect.
[We're not adding something to an API for example.]
It looks good enough to me (and in such areas I like giving people
some freedom).