Re: [RFA 1/2] Linespec rewrite (update 2)

[Tom Tromey <tromey@redhat.com>]

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

Keith> Tom requested a repost of this patch, so here it is.

Thanks, Keith.

This patch is pretty hard to review.  I've been reading the branch
instead, but even there I find it hard to wrap my head around it all.

That said, I like what I see.  Congratulations on conquering linespec.

I'm glad to see others testing this patch on their various scenarios, I
think that raises the confidence level in the patch.

Keith> +const char * const linespec_keywords[] = { "if", "thread", "task" };

Should also be static.

Keith> +      /* If we're in list mode, and the next token is a string beginning
Keith> +	 with ",", we're dealing with a ranged listing.  Stop parsing
Keith> +	 and return.  */
Keith> +      if (PARSER_STATE (parser)->list_mode
Keith> +	  && token.type == LSTOKEN_STRING
Keith> +	  && *LS_TOKEN_STOKEN (token).ptr == ',')
Keith> +	return;

I thought that historically a top-level comma always terminated a
linespec -- not just in list mode.  It should be possible to write a
test case for this in Python pretty easily.

Keith> +static void
Keith> +canonicalize_linespec (struct linespec_state *state, linespec_t ls)
[...]
Keith> +      if (ls->line_offset.sign != unknown)
Keith> +	{
Keith> +	  if (need_colon)
Keith> +	    fputc_unfiltered (':', buf);
Keith> +	  fprintf_filtered (buf, "%s%d",
Keith> +			    (ls->line_offset.sign == none ? ""
Keith> +			     : ls->line_offset.sign == plus ? "+" : "-"),
Keith> +			    ls->line_offset.offset);

I am curious when this code can trigger.
Can we end up with a canonical form like "function:+5"?
I was hoping to reserve that syntax for a later addition; and anyway in
general I think relative linespecs need to be made absolute by the
canonicalization process, since otherwise re-setting won't do the right
thing.

Keith> +      /* We have an expression.  No other attribute is allowed.  */

It would be helpful if the constraints on the fields of 'struct
linespec' were documented there.

Keith> +	  pspace = elem->minsym->ginfo.obj_section->objfile->pspace;

Should use SYMBOL_OBJ_SECTION.  I didn't audit for other instances.

Keith> +  else if (ls->minimal_symbols != NULL)
Keith> +    {
Keith> +      /* We found minimal symbols, but no normal symbols.  */
Keith> +      int i;
Keith> +      minsym_and_objfile_d *elem;
Keith> +
Keith> +      for (i = 0;
Keith> +	   VEC_iterate (minsym_and_objfile_d, ls->minimal_symbols, i, elem);
Keith> +	   ++i)
Keith> +	minsym_found (state, elem->objfile, elem->minsym, &sals);

Why are minsyms sorted by pspace in one branch but not another?

Keith> +++ b/gdb/testsuite/gdb.cp/cplabel.exp
[...]
Keith> +if {[prepare_for_testing "$testfile.exp" $testfile $srcfile]} {
Keith> +    return -1

I suspect this needs a skip_cplus_tests check.

Tom