Re: [PATCH v4 7/9] Explicit locations: add UI features for CLI

[Doug Evans <xdje42 at gmail dot com>]

Keith Seitz <keiths@redhat.com> writes:
>>> +/* See description in location.h.  */
>>> +
>>> +struct event_location *
>>> +string_to_explicit_location (const char **argp,
>>> +			     const struct language_defn *language,
>>> +			     int dont_throw)
>>> +{
>>> +  struct cleanup *cleanup;
>>> +  struct event_location *location;
>>> +
>>> +  /* It is assumed that input beginning with '-' and a non-digit
>>> +     character is an explicit location.  */
>>> +  if (argp == NULL
>>> +      || *argp == '\0'
>>> +      || *argp[0] != '-'
>>> +      || !isalpha ((*argp)[1]))
>>> +    return NULL;
>>> +
>>> +  location = new_explicit_location (NULL);
>>> +  cleanup = make_cleanup_delete_event_location (location);
>>> +
>>> +  /* Process option/argument pairs.  dprintf_command
>>> +     requires that processing stop on ','.  */
>>> +  while ((*argp)[0] != '\0' && (*argp)[0] != ',')
>>> +    {
>>> +      int len;
>>> +      char *opt, *oarg;
>>> +      const char *start;
>>> +      struct cleanup *inner;
>>> +
>>> +      /* If *ARGP starts with a keyword, stop processing
>>> +	 options.  */
>>> +      if (linespec_lexer_lex_keyword (*argp) != NULL)
>>> +	break;
>>> +
>>> +      /* Mark the start of the string in case we need to rewind.  */
>>> +      start = *argp;
>>> +
>>> +      /* Get the option string.  */
>>> +      opt = explicit_location_lex_one (argp, language);
>>> +      inner = make_cleanup (xfree, opt);
>>> +
>>> +      *argp = skip_spaces_const (*argp);
>>> +
>>> +      /* Get the argument string.  */
>>> +      oarg = explicit_location_lex_one (argp, language);
>>> +
>>> +      *argp = skip_spaces_const (*argp);
>>> +
>>> +      /* Use the length of the option to allow abbreviations.  */
>>> +      len = strlen (opt);
>>> +
>>> +      /* All options have a required argument.  Checking for this required
>>> +	 argument is deferred until later.  */
>>> +      if (strncmp (opt, "-source", len) == 0)
>>> +	EL_EXPLICIT (location)->source_filename = oarg;
>>> +      else if (strncmp (opt, "-function", len) == 0)
>>> +	EL_EXPLICIT (location)->function_name = oarg;
>>> +      else if (strncmp (opt, "-line", len) == 0)
>>> +	{
>>> +	  if (oarg != NULL)
>>> +	    {
>>> +	      TRY
>>> +		{
>>> +		  EL_EXPLICIT (location)->line_offset
>>> +		    = linespec_parse_line_offset (oarg);
>>> +		}
>>> +	      CATCH (e, RETURN_MASK_ERROR)
>>> +		{
>>> +		  xfree (oarg);
>> 
>> Could other exception types leak oarg here?
>
> Not that I can see. When any successful argument value is parsed, it is
> added to the event_location, which has a cleanup on it which will free
> any defined members when an exception occurs.
>
> The only two functions (in this loop) that could throw an exception are
> explicit_location_lex_one and linespec_parse_line_offset.
>
> In the former case, the option name has a cleanup when parsing the
> value. The value is either saved into the event_location or discarded if
> we are going to throw an exception.
>
> linespec_parse_line_offset can throw an error (GENERIC_ERROR), but it is
> already caught and memory for oarg is freed. Nothing can generate a
> RETURN_QUIT as far as I can tell. Did you have a case specifically in mind?

I'm just worried about the robustness (or lack thereof)
to future changes.

Plus the readability.
You've done the research to conclude that the code is ok,
(e.g., linespec_parse_line_offset doesn't call QUIT)
but the casual reader will be left with the question:
"Hmmm, can oarg leak?", and a conscientious reader would then
have to put in the time to do that research too.
It'd be preferable if the code could be read and
understood to be correct without having to do that research.