Re: [RFA 3/5] Explicit linespecs - implementation

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> Ok, so here's what everyone has been waiting for. This patch adds the
Keith> implementation of explicit linespecs, including CLI and MI support.

Thanks, Keith.  I'm very encouraged by this project.

Keith> +/* Throw an exception for unsupported explicit linespec.  */
Keith> +
Keith> +static void ATTRIBUTE_NORETURN
Keith> +explicit_linespec_unsupported (enum bptype type)
Keith> +{
Keith> +  error (_("explicit linespec not supported for %s"), bptype_string (type));

I found it pretty hard to track through the code to find out under what
circumstances this can really be called.  Is it really an error for
users?  Or should it be an assertion?

Keith> +      char *where = b->addr_string;
Keith> +      struct cleanup *free_where = make_cleanup (null_cleanup, NULL);
Keith> +
Keith> +      if (b->explicit != NULL)
Keith> +	{
Keith> +	  where = explicit_linespec_to_string (b->explicit, b->addr_string);
Keith> +	  make_cleanup (xfree, where);
Keith> +	}
Keith> +
Keith> +      printf_filtered (_(" (%s) pending."), where);
Keith> +      do_cleanups (free_where);

I think it is better for users to preserve what they typed, warts and
all, and just regurgitate that.

Once upon a time we did not do this for breakpoint conditions -- we
printed from the expression object instead.  This turned out to be quite
confusing since you would enter:

   cond 5 p == 0x2168bf0

and 'info b' would print

  if p == 34996784

... which is hard to recognize as the same thing, more difficult to
search for, etc.

I think this also means that if the user enters an explicit linespec, it
will be shown as a normal linespec, which seems sub-optimal.

Keith> +static char *
Keith> +explicit_lex_one (char **inp)
[...]
Keith> +	      const char *end = skip_quote_char (start + 1, quote_char);

IIRC, skip_quote_char has a funny quoting style -- there's no way to
quote a quote.

I think for new code we ought to adopt a better convention, like what
buildargv does.

Keith> +	      /* Get the argument string.  */
Keith> +	      oarg = explicit_lex_one (argp);
Keith> +	      make_cleanup (xfree, oarg);
[...]
Keith> +	      else if (strncmp (opt, "-address", len) == 0)
Keith> +		els->expression = oarg;

This means that users will have to type:

    break -address "complicated expression (\"string\")"

Note how the string has to be quoted.
That seems not very user-friendly.

It would be nicer to parse the expression after -address, but that has
other complications -- for one thing, it means that -address would have
to come last, since parsing expression is "funny".

I think that is the better tradeoff though, because reordering the
arguments is less painful than quoting expressions.  Also I wonder
whether completion works in a quoted expression...


A nice follow-up patch would be to expose this to the Python breakpoint
constructor, say via keyword arguments.

Tom