Re: [PATCH] Fix for PR15117

[Keith Seitz <keiths at redhat dot com>]

This looks great! Thank you for the expanded comments. That cleared a 
misconception I had. I was incorrect asserting that your convenience 
variable change was not located properly.

On 08/02/2013 05:14 AM, ali_anwar wrote:
> There are already test cases in the gdb.base/break.exp for this scenario
> (break $foo) and they did get pass. The undefined scenario is already
> covered in gdb.linespec/ls-errs.exp and all test cases in gdb.linespec/*
> got passed.
[snip]

Great! Thank you for pointing that out.

Two tiny nits:

> Index: linespec.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/linespec.c,v
> retrieving revision 1.185
> diff -u -r1.185 linespec.c
> --- linespec.c	30 May 2013 16:57:38 -0000	1.185
> +++ linespec.c	2 Aug 2013 11:43:57 -0000
> @@ -1649,7 +1649,7 @@
>     else
>       {
>         /* NAME was not a function or a method.  So it must be a label
> -	 name.  */
> +	 name or user specified variable like "break foo.c:$zippo".  */
>         labels = find_label_symbols (PARSER_STATE (parser), NULL,
>   				   &symbols, name);
>         if (labels != NULL)
> @@ -1660,6 +1660,22 @@
>   	  symbols = NULL;
>   	  discard_cleanups (cleanup);
>   	}
> +      else if (token.type == LSTOKEN_STRING
> +	       && *LS_TOKEN_STOKEN (token).ptr == '$')
> +	{
> +	  /* User specified a convenience variable or history value.  */
> +	  PARSER_RESULT (parser)->line_offset
> +	    = linespec_parse_variable (PARSER_STATE (parser), name);
> +
> +	  if (PARSER_RESULT (parser)->line_offset.sign == LINE_OFFSET_UNKNOWN)
> +	    {
> +	      /* Not able to parse user specified variable. Do not
> +		 throw an error here. parse_linespec will do it for us*/

This comment is not formatted properly: two spaces after '.'. Use 
complete sentences (where possible/feasible). This should probably read:

/* The user-specified variable was not valid.  Do not
   throw an error here.  parse_linespec will do it for us.  */

[i.e, just copy the bits from the following block]

> +	      PARSER_RESULT (parser)->function_name = name;
> +	      discard_cleanups (cleanup);
> +	      return;
> +	    }
> +	}
>         else
>   	{
>   	  /* The name is also not a label.  Abort parsing.  Do not throw
> Index: testsuite/gdb.base/break.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/break.exp,v
> retrieving revision 1.58
> diff -u -r1.58 break.exp
> --- testsuite/gdb.base/break.exp	7 Jun 2013 17:31:07 -0000	1.58
> +++ testsuite/gdb.base/break.exp	2 Aug 2013 11:54:40 -0000
> @@ -957,6 +957,18 @@
>       }
>   }
>
> +#
> +# Test break via convenience variable with file name
> +#
> +set line [gdb_get_line_number "set breakpoint 1 here"]
> +gdb_test_no_output "set \$l = $line"

I'm showing extra whitespace at the end of the above line.  Could you 
double-check that before committing?

> +gdb_breakpoint ${srcfile}:\$l
> +
> +gdb_test_no_output "set \$foo=81.5" \
> +    "set convenience variable \$foo to 81.5"
> +gdb_test "break $srcfile:\$foo" \
> +    "Convenience variables used in line specs must have integer values.*" \
> +    "set breakpoint via non-integer convenience variable disallowed"
>
>   # Reset the default arguments for VxWorks
>   if [istarget "*-*-vxworks*"] {

With those trivial things fixed, I think this patch is ready for a 
global maintainer to review and approve.

Thanks again!
Keith