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

[Doug Evans <xdje42 at gmail dot com>]

Keith Seitz <keiths@redhat.com> writes:
> Doug Evans <dje@google.com> writes:
>
>> As for reposting, in this particular case (2/9) I suppose you should,
>> but only for informational sake. No need for waiting for another review.
>
> For the record, this is the version I plan to commit/push.  The only change
> is the removal of event_location_to_string_cosnt, which was not being used
> anymore.
>
> gdb/ChangeLog:
>
> 	* Makefile.in (SFILES): Add location.c.
> 	(HFILES_NO_SRCDIR): Add location.h.
> 	(COMMON_OBS): Add location.o.
> 	* linespec.c (linespec_lex_to_end): New function.
> 	* linespec.h (linespec_lex_to_end): Declare.
> 	* location.c: New file.
> 	* location.h: New file.

Alas the removal of event_location_to_string_const isn't quite right.

> +/* See description in location.h.  */
> +
> +const char *
> +event_location_to_string (struct event_location *location)
> +{
> +  char *result = NULL;
> +
> +  if (EL_STRING (location) != NULL)
> +    return xstrdup (EL_STRING (location));
> +
> +  switch (EL_TYPE (location))
> +    {
> +    case LINESPEC_LOCATION:
> +      if (EL_LINESPEC (location) != NULL)
> +	result = xstrdup (EL_LINESPEC (location));
> +      break;
> +
> +    default:
> +      gdb_assert_not_reached ("unknown event location type");
> +    }
> +
> +  return result;
> +}

The caching done by the previous version is lost:

+/* See description in location.h.  */
+
+const char *
+event_location_to_string (struct event_location *location)
+{
+  /* Cache a copy of the string representation.  */
+  if (EL_STRING (location) == NULL)
+    EL_STRING (location) = event_location_to_string_const (location);
+
+  return EL_STRING (location);
+}

The result is still owned by the location so we don't want to return
an xstrdup'd copy.

How about:

const char *
event_location_to_string (const struct event_location *location)
{
  if (EL_STRING (location) == NULL)
    {
      switch (EL_TYPE (location))
	{
	case LINESPEC_LOCATION:
	  if (EL_LINESPEC (location) != NULL)
	    EL_STRING (location) = xstrdup (EL_LINESPEC (location));
	  break;

	default:
	  gdb_assert_not_reached ("unknown event location type");
	}
    }

  return EL_STRING (location);
}