Re: [PATCH v3 2/9] Explicit locations: introduce new struct event_location-based API

[Keith Seitz <keiths at redhat dot com>]

On 03/01/2015 12:14 PM, Doug Evans wrote:
> Keith Seitz <keiths@redhat.com> writes:
>>
>> +/* See description in linespec.h.  */
>> +
>> +void
>> +linespec_lex_to_end (char **stringp)
>> +{
>> +  linespec_parser parser;
>> +  struct cleanup *cleanup;
>> +  linespec_token token;
>> +  volatile struct gdb_exception e;
>> +  const char *p, *orig;
>> +
>> +  if (stringp == NULL || *stringp == NULL)
>> +    return;
>> +
>> +  linespec_parser_new (&parser, 0, current_language, NULL, 0, NULL);
>> +  cleanup = make_cleanup (linespec_parser_delete, &parser);
>> +  parser.lexer.saved_arg = *stringp;
>> +  parser.keyword_ok = 1;
>
> ====
> parser.keyword_ok = 0; ?

Nope. Not in this case. We need to stop lexing when we find a keyword.
Otherwise, default locations will break. From signest.exp:

(gdb) break if 0
Function "if 0" not defined.
Make breakpoint pending on future shared library load? (y or [n])

>> +  orig = p = *stringp;
>> +  parser.lexer.stream = &p;
>> +
>> +  TRY_CATCH (e, RETURN_MASK_ERROR)
>> +    {
>> +      do
>> +	{
>> +	  /* Stop before any comma tokens;  we need it to keep it
>> +	     as the next token in the string.  */
>> +	  token = linespec_lexer_peek_token (&parser);
>> +	  if (token.type == LSTOKEN_COMMA)
>> +	    break;
>> +
>> +	  /* For addresses advance the parser stream past
>> +	     any parsed input and stop lexing.  */
>> +	  if (token.type == LSTOKEN_STRING
>> +	      && *LS_TOKEN_STOKEN (token).ptr == '*')
>
> ====
> Will this mis-parse an address appearing after the first token?
> [IOW, *address must be the first token, right?]

That's right. I have made no attempt to change the current behavior. But
it doesn't really matter in this context. The chunk dealing with address
locations is removed from this function in the
explicit-address-locations patch. [It causes no regressions in the test
suite, so buildbot should not report any, either.]

>> +	    {
>> +	      const char *arg, *orig;
>> +
>> +	      orig = arg = *stringp;
>> +	      (void) linespec_expression_to_pc (&arg);
>> +	      PARSER_STREAM (&parser) += arg - orig;
>
> ====
> How about: PARSER_STREAM (&parser) = arg; ?

Sure. [NOTE: This disappears in subsequent patch.]

>> +	      break;
>> +	    }
>> +
>> +	  token = linespec_lexer_consume_token (&parser);
>> +
>> +	  /* Keywords are okay after the first token.  */
>> +	  parser.keyword_ok = 1;
>> +	}
>> +      while (token.type != LSTOKEN_EOI && token.type != LSTOKEN_KEYWORD);
>> +    }
>> +
>> +  *stringp += PARSER_STREAM (&parser) - orig;
>
> ====
> Similary, how about: *stringp = PARSER_STREAM (&parser); ?
>

Unfortunately, we cannot do this. PARSER_STREAM is const, stringp is
not. Well, we probably could do this, but it would require
constification of a lot of other functions/files. I'd be happy to
investigate that as a follow-up patch.

I'll have a new series posted here in a bit.

Keith