* [PATCH 0/6] C++-ify location.c
@ 2022-01-14 16:50 Tom Tromey
2022-01-14 16:50 ` [PATCH 1/6] Remove a use of xfree in location.c Tom Tromey
` (6 more replies)
0 siblings, 7 replies; 10+ messages in thread
From: Tom Tromey @ 2022-01-14 16:50 UTC (permalink / raw)
To: gdb-patches
My foray into breakpoint.c led to me location.c. This patch cleans it
up a bit, by C++-ifying the code. This removes some explicit memory
management and removes some copies. It also, I think, reduces the
chances for errors here by removing the use of a union in favor of
subclassing.
This code could be cleaned up a bit more. For example,
explicit_location claims its fields are malloc'd, but this is not
universally the case. I haven't attempted that.
Regression tested on x86-64 Fedora 34.
Tom
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/6] Remove a use of xfree in location.c
2022-01-14 16:50 [PATCH 0/6] C++-ify location.c Tom Tromey
@ 2022-01-14 16:50 ` Tom Tromey
2022-01-14 16:50 ` [PATCH 2/6] Boolify explicit_to_string_internal Tom Tromey
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2022-01-14 16:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This small cleanup removes a use of xfree from location.c, by
switching to unique_xmalloc_ptr. One function is only used in
location.c, so it is made static. And, another function is changed to
avoid a copy.
---
gdb/linespec.c | 8 ++------
gdb/location.c | 14 +++++++-------
gdb/location.h | 14 ++++----------
3 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/gdb/linespec.c b/gdb/linespec.c
index eb911ccdb35..b24cf30dc71 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2098,12 +2098,8 @@ canonicalize_linespec (struct linespec_state *state, const linespec *ls)
/* If this location originally came from a linespec, save a string
representation of it for display and saving to file. */
if (state->is_linespec)
- {
- char *linespec = explicit_location_to_linespec (explicit_loc);
-
- set_event_location_string (canon, linespec);
- xfree (linespec);
- }
+ set_event_location_string (canon,
+ explicit_location_to_linespec (explicit_loc));
}
/* Given a line offset in LS, construct the relevant SALs. */
diff --git a/gdb/location.c b/gdb/location.c
index 45f1d59ad81..d4180cfe429 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -231,7 +231,7 @@ get_explicit_location_const (const struct event_location *location)
AS_LINESPEC is non-zero if this string should be a linespec.
Otherwise it will be output in explicit form. */
-static char *
+static gdb::unique_xmalloc_ptr<char>
explicit_to_string_internal (int as_linespec,
const struct explicit_location *explicit_loc)
{
@@ -282,12 +282,12 @@ explicit_to_string_internal (int as_linespec,
explicit_loc->line_offset.offset);
}
- return xstrdup (buf.c_str ());
+ return make_unique_xstrdup (buf.c_str ());
}
/* See description in location.h. */
-char *
+static gdb::unique_xmalloc_ptr<char>
explicit_location_to_string (const struct explicit_location *explicit_loc)
{
return explicit_to_string_internal (0, explicit_loc);
@@ -295,7 +295,7 @@ explicit_location_to_string (const struct explicit_location *explicit_loc)
/* See description in location.h. */
-char *
+gdb::unique_xmalloc_ptr<char>
explicit_location_to_linespec (const struct explicit_location *explicit_loc)
{
return explicit_to_string_internal (1, explicit_loc);
@@ -425,7 +425,7 @@ event_location_to_string (struct event_location *location)
case EXPLICIT_LOCATION:
EL_STRING (location)
- = explicit_location_to_string (EL_EXPLICIT (location));
+ = explicit_location_to_string (EL_EXPLICIT (location)).release ();
break;
case PROBE_LOCATION:
@@ -981,8 +981,8 @@ event_location_empty_p (const struct event_location *location)
void
set_event_location_string (struct event_location *location,
- const char *string)
+ gdb::unique_xmalloc_ptr<char> string)
{
xfree (EL_STRING (location));
- EL_STRING (location) = string == NULL ? NULL : xstrdup (string);
+ EL_STRING (location) = string.release ();
}
diff --git a/gdb/location.h b/gdb/location.h
index c6e1402d762..b391ce3b1dd 100644
--- a/gdb/location.h
+++ b/gdb/location.h
@@ -109,16 +109,10 @@ struct explicit_location
extern enum event_location_type
event_location_type (const struct event_location *);
-/* Return a malloc'd explicit string representation of the given
- explicit location. The location must already be canonicalized/valid. */
+/* Return a linespec string representation of the given explicit
+ location. The location must already be canonicalized/valid. */
-extern char *
- explicit_location_to_string (const struct explicit_location *explicit_loc);
-
-/* Return a malloc'd linespec string representation of the given
- explicit location. The location must already be canonicalized/valid. */
-
-extern char *
+extern gdb::unique_xmalloc_ptr<char>
explicit_location_to_linespec (const struct explicit_location *explicit_loc);
/* Return a string representation of the LOCATION.
@@ -286,6 +280,6 @@ extern int event_location_empty_p (const struct event_location *location);
extern void
set_event_location_string (struct event_location *location,
- const char *string);
+ gdb::unique_xmalloc_ptr<char> string);
#endif /* LOCATION_H */
--
2.31.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/6] Boolify explicit_to_string_internal
2022-01-14 16:50 [PATCH 0/6] C++-ify location.c Tom Tromey
2022-01-14 16:50 ` [PATCH 1/6] Remove a use of xfree in location.c Tom Tromey
@ 2022-01-14 16:50 ` Tom Tromey
2022-01-14 16:50 ` [PATCH 3/6] Remove EL_* macros from location.c Tom Tromey
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2022-01-14 16:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This changes explicit_to_string_internal to use 'bool' rather than
'int'.
---
gdb/location.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/gdb/location.c b/gdb/location.c
index d4180cfe429..35ca2ac71b8 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -228,14 +228,14 @@ get_explicit_location_const (const struct event_location *location)
/* This convenience function returns a malloc'd string which
represents the location in EXPLICIT_LOC.
- AS_LINESPEC is non-zero if this string should be a linespec.
+ AS_LINESPEC is true if this string should be a linespec.
Otherwise it will be output in explicit form. */
static gdb::unique_xmalloc_ptr<char>
-explicit_to_string_internal (int as_linespec,
+explicit_to_string_internal (bool as_linespec,
const struct explicit_location *explicit_loc)
{
- int need_space = 0;
+ bool need_space = false;
char space = as_linespec ? ':' : ' ';
string_file buf;
@@ -244,7 +244,7 @@ explicit_to_string_internal (int as_linespec,
if (!as_linespec)
buf.puts ("-source ");
buf.puts (explicit_loc->source_filename);
- need_space = 1;
+ need_space = true;
}
if (explicit_loc->function_name != NULL)
@@ -256,7 +256,7 @@ explicit_to_string_internal (int as_linespec,
if (!as_linespec)
buf.puts ("-function ");
buf.puts (explicit_loc->function_name);
- need_space = 1;
+ need_space = true;
}
if (explicit_loc->label_name != NULL)
@@ -266,7 +266,7 @@ explicit_to_string_internal (int as_linespec,
if (!as_linespec)
buf.puts ("-label ");
buf.puts (explicit_loc->label_name);
- need_space = 1;
+ need_space = true;
}
if (explicit_loc->line_offset.sign != LINE_OFFSET_UNKNOWN)
@@ -290,7 +290,7 @@ explicit_to_string_internal (int as_linespec,
static gdb::unique_xmalloc_ptr<char>
explicit_location_to_string (const struct explicit_location *explicit_loc)
{
- return explicit_to_string_internal (0, explicit_loc);
+ return explicit_to_string_internal (false, explicit_loc);
}
/* See description in location.h. */
@@ -298,7 +298,7 @@ explicit_location_to_string (const struct explicit_location *explicit_loc)
gdb::unique_xmalloc_ptr<char>
explicit_location_to_linespec (const struct explicit_location *explicit_loc)
{
- return explicit_to_string_internal (1, explicit_loc);
+ return explicit_to_string_internal (true, explicit_loc);
}
/* See description in location.h. */
--
2.31.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/6] Remove EL_* macros from location.c
2022-01-14 16:50 [PATCH 0/6] C++-ify location.c Tom Tromey
2022-01-14 16:50 ` [PATCH 1/6] Remove a use of xfree in location.c Tom Tromey
2022-01-14 16:50 ` [PATCH 2/6] Boolify explicit_to_string_internal Tom Tromey
@ 2022-01-14 16:50 ` Tom Tromey
2022-01-14 16:50 ` [PATCH 4/6] Split event_location into subclasses Tom Tromey
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2022-01-14 16:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This patch removes the old-style EL_* macros from location.c. This
cleans up the code by itself, IMO, but also enables further cleanups
in subsequent patches.
---
gdb/location.c | 183 ++++++++++++++++++++++++-------------------------
1 file changed, 90 insertions(+), 93 deletions(-)
diff --git a/gdb/location.c b/gdb/location.c
index 35ca2ac71b8..9c33ea4746e 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -37,31 +37,25 @@ struct event_location
{
/* The type of this breakpoint specification. */
enum event_location_type type;
-#define EL_TYPE(P) (P)->type
union
{
/* A probe. */
char *addr_string;
-#define EL_PROBE(P) ((P)->u.addr_string)
/* A "normal" linespec. */
struct linespec_location linespec_location;
-#define EL_LINESPEC(P) (&(P)->u.linespec_location)
/* An address in the inferior. */
CORE_ADDR address;
-#define EL_ADDRESS(P) (P)->u.address
/* An explicit location. */
struct explicit_location explicit_loc;
-#define EL_EXPLICIT(P) (&((P)->u.explicit_loc))
} u;
/* Cached string representation of this location. This is used, e.g., to
save stop event locations to file. Malloc'd. */
char *as_string;
-#define EL_STRING(P) ((P)->as_string)
};
/* See description in location.h. */
@@ -69,7 +63,7 @@ struct event_location
enum event_location_type
event_location_type (const struct event_location *location)
{
- return EL_TYPE (location);
+ return location->type;
}
/* See description in location.h. */
@@ -91,8 +85,8 @@ new_linespec_location (const char **linespec,
struct event_location *location;
location = XCNEW (struct event_location);
- EL_TYPE (location) = LINESPEC_LOCATION;
- EL_LINESPEC (location)->match_type = match_type;
+ location->type = LINESPEC_LOCATION;
+ location->u.linespec_location.match_type = match_type;
if (*linespec != NULL)
{
const char *p;
@@ -101,7 +95,8 @@ new_linespec_location (const char **linespec,
linespec_lex_to_end (linespec);
p = remove_trailing_whitespace (orig, *linespec);
if ((p - orig) > 0)
- EL_LINESPEC (location)->spec_string = savestring (orig, p - orig);
+ location->u.linespec_location.spec_string
+ = savestring (orig, p - orig);
}
return event_location_up (location);
}
@@ -111,8 +106,8 @@ new_linespec_location (const char **linespec,
const linespec_location *
get_linespec_location (const struct event_location *location)
{
- gdb_assert (EL_TYPE (location) == LINESPEC_LOCATION);
- return EL_LINESPEC (location);
+ gdb_assert (location->type == LINESPEC_LOCATION);
+ return &location->u.linespec_location;
}
/* See description in location.h. */
@@ -124,10 +119,10 @@ new_address_location (CORE_ADDR addr, const char *addr_string,
struct event_location *location;
location = XCNEW (struct event_location);
- EL_TYPE (location) = ADDRESS_LOCATION;
- EL_ADDRESS (location) = addr;
+ location->type = ADDRESS_LOCATION;
+ location->u.address = addr;
if (addr_string != NULL)
- EL_STRING (location) = xstrndup (addr_string, addr_string_len);
+ location->as_string = xstrndup (addr_string, addr_string_len);
return event_location_up (location);
}
@@ -136,8 +131,8 @@ new_address_location (CORE_ADDR addr, const char *addr_string,
CORE_ADDR
get_address_location (const struct event_location *location)
{
- gdb_assert (EL_TYPE (location) == ADDRESS_LOCATION);
- return EL_ADDRESS (location);
+ gdb_assert (location->type == ADDRESS_LOCATION);
+ return location->u.address;
}
/* See description in location.h. */
@@ -145,8 +140,8 @@ get_address_location (const struct event_location *location)
const char *
get_address_string_location (const struct event_location *location)
{
- gdb_assert (EL_TYPE (location) == ADDRESS_LOCATION);
- return EL_STRING (location);
+ gdb_assert (location->type == ADDRESS_LOCATION);
+ return location->as_string;
}
/* See description in location.h. */
@@ -157,9 +152,9 @@ new_probe_location (const char *probe)
struct event_location *location;
location = XCNEW (struct event_location);
- EL_TYPE (location) = PROBE_LOCATION;
+ location->type = PROBE_LOCATION;
if (probe != NULL)
- EL_PROBE (location) = xstrdup (probe);
+ location->u.addr_string = xstrdup (probe);
return event_location_up (location);
}
@@ -168,8 +163,8 @@ new_probe_location (const char *probe)
const char *
get_probe_location (const struct event_location *location)
{
- gdb_assert (EL_TYPE (location) == PROBE_LOCATION);
- return EL_PROBE (location);
+ gdb_assert (location->type == PROBE_LOCATION);
+ return location->u.addr_string;
}
/* See description in location.h. */
@@ -180,28 +175,28 @@ new_explicit_location (const struct explicit_location *explicit_loc)
struct event_location tmp;
memset (&tmp, 0, sizeof (struct event_location));
- EL_TYPE (&tmp) = EXPLICIT_LOCATION;
- initialize_explicit_location (EL_EXPLICIT (&tmp));
+ tmp.type = EXPLICIT_LOCATION;
+ initialize_explicit_location (&tmp.u.explicit_loc);
if (explicit_loc != NULL)
{
- EL_EXPLICIT (&tmp)->func_name_match_type
+ tmp.u.explicit_loc.func_name_match_type
= explicit_loc->func_name_match_type;
if (explicit_loc->source_filename != NULL)
{
- EL_EXPLICIT (&tmp)->source_filename
+ tmp.u.explicit_loc.source_filename
= explicit_loc->source_filename;
}
if (explicit_loc->function_name != NULL)
- EL_EXPLICIT (&tmp)->function_name
+ tmp.u.explicit_loc.function_name
= explicit_loc->function_name;
if (explicit_loc->label_name != NULL)
- EL_EXPLICIT (&tmp)->label_name = explicit_loc->label_name;
+ tmp.u.explicit_loc.label_name = explicit_loc->label_name;
if (explicit_loc->line_offset.sign != LINE_OFFSET_UNKNOWN)
- EL_EXPLICIT (&tmp)->line_offset = explicit_loc->line_offset;
+ tmp.u.explicit_loc.line_offset = explicit_loc->line_offset;
}
return copy_event_location (&tmp);
@@ -212,8 +207,8 @@ new_explicit_location (const struct explicit_location *explicit_loc)
struct explicit_location *
get_explicit_location (struct event_location *location)
{
- gdb_assert (EL_TYPE (location) == EXPLICIT_LOCATION);
- return EL_EXPLICIT (location);
+ gdb_assert (location->type == EXPLICIT_LOCATION);
+ return &location->u.explicit_loc;
}
/* See description in location.h. */
@@ -221,8 +216,8 @@ get_explicit_location (struct event_location *location)
const struct explicit_location *
get_explicit_location_const (const struct event_location *location)
{
- gdb_assert (EL_TYPE (location) == EXPLICIT_LOCATION);
- return EL_EXPLICIT (location);
+ gdb_assert (location->type == EXPLICIT_LOCATION);
+ return &location->u.explicit_loc;
}
/* This convenience function returns a malloc'd string which
@@ -309,44 +304,46 @@ copy_event_location (const struct event_location *src)
struct event_location *dst;
dst = XCNEW (struct event_location);
- EL_TYPE (dst) = EL_TYPE (src);
- if (EL_STRING (src) != NULL)
- EL_STRING (dst) = xstrdup (EL_STRING (src));
+ dst->type = src->type;
+ if (src->as_string != NULL)
+ dst->as_string = xstrdup (src->as_string);
- switch (EL_TYPE (src))
+ switch (src->type)
{
case LINESPEC_LOCATION:
- EL_LINESPEC (dst)->match_type = EL_LINESPEC (src)->match_type;
- if (EL_LINESPEC (src)->spec_string != NULL)
- EL_LINESPEC (dst)->spec_string
- = xstrdup (EL_LINESPEC (src)->spec_string);
+ dst->u.linespec_location.match_type
+ = src->u.linespec_location.match_type;
+ if (src->u.linespec_location.spec_string != NULL)
+ dst->u.linespec_location.spec_string
+ = xstrdup (src->u.linespec_location.spec_string);
break;
case ADDRESS_LOCATION:
- EL_ADDRESS (dst) = EL_ADDRESS (src);
+ dst->u.address = src->u.address;
break;
case EXPLICIT_LOCATION:
- EL_EXPLICIT (dst)->func_name_match_type
- = EL_EXPLICIT (src)->func_name_match_type;
- if (EL_EXPLICIT (src)->source_filename != NULL)
- EL_EXPLICIT (dst)->source_filename
- = xstrdup (EL_EXPLICIT (src)->source_filename);
+ dst->u.explicit_loc.func_name_match_type
+ = src->u.explicit_loc.func_name_match_type;
+ if (src->u.explicit_loc.source_filename != NULL)
+ dst->u.explicit_loc.source_filename
+ = xstrdup (src->u.explicit_loc.source_filename);
- if (EL_EXPLICIT (src)->function_name != NULL)
- EL_EXPLICIT (dst)->function_name
- = xstrdup (EL_EXPLICIT (src)->function_name);
+ if (src->u.explicit_loc.function_name != NULL)
+ dst->u.explicit_loc.function_name
+ = xstrdup (src->u.explicit_loc.function_name);
- if (EL_EXPLICIT (src)->label_name != NULL)
- EL_EXPLICIT (dst)->label_name = xstrdup (EL_EXPLICIT (src)->label_name);
+ if (src->u.explicit_loc.label_name != NULL)
+ dst->u.explicit_loc.label_name
+ = xstrdup (src->u.explicit_loc.label_name);
- EL_EXPLICIT (dst)->line_offset = EL_EXPLICIT (src)->line_offset;
+ dst->u.explicit_loc.line_offset = src->u.explicit_loc.line_offset;
break;
case PROBE_LOCATION:
- if (EL_PROBE (src) != NULL)
- EL_PROBE (dst) = xstrdup (EL_PROBE (src));
+ if (src->u.addr_string != NULL)
+ dst->u.addr_string = xstrdup (src->u.addr_string);
break;
default:
@@ -361,12 +358,12 @@ event_location_deleter::operator() (event_location *location) const
{
if (location != NULL)
{
- xfree (EL_STRING (location));
+ xfree (location->as_string);
- switch (EL_TYPE (location))
+ switch (location->type)
{
case LINESPEC_LOCATION:
- xfree (EL_LINESPEC (location)->spec_string);
+ xfree (location->u.linespec_location.spec_string);
break;
case ADDRESS_LOCATION:
@@ -374,13 +371,13 @@ event_location_deleter::operator() (event_location *location) const
break;
case EXPLICIT_LOCATION:
- xfree (EL_EXPLICIT (location)->source_filename);
- xfree (EL_EXPLICIT (location)->function_name);
- xfree (EL_EXPLICIT (location)->label_name);
+ xfree (location->u.explicit_loc.source_filename);
+ xfree (location->u.explicit_loc.function_name);
+ xfree (location->u.explicit_loc.label_name);
break;
case PROBE_LOCATION:
- xfree (EL_PROBE (location));
+ xfree (location->u.addr_string);
break;
default:
@@ -396,40 +393,40 @@ event_location_deleter::operator() (event_location *location) const
const char *
event_location_to_string (struct event_location *location)
{
- if (EL_STRING (location) == NULL)
+ if (location->as_string == NULL)
{
- switch (EL_TYPE (location))
+ switch (location->type)
{
case LINESPEC_LOCATION:
- if (EL_LINESPEC (location)->spec_string != NULL)
+ if (location->u.linespec_location.spec_string != NULL)
{
- linespec_location *ls = EL_LINESPEC (location);
+ linespec_location *ls = &location->u.linespec_location;
if (ls->match_type == symbol_name_match_type::FULL)
{
- EL_STRING (location)
+ location->as_string
= concat ("-qualified ", ls->spec_string, (char *) NULL);
}
else
- EL_STRING (location) = xstrdup (ls->spec_string);
+ location->as_string = xstrdup (ls->spec_string);
}
break;
case ADDRESS_LOCATION:
{
const char *addr_string
- = core_addr_to_string (EL_ADDRESS (location));
- EL_STRING (location)
+ = core_addr_to_string (location->u.address);
+ location->as_string
= xstrprintf ("*%s", addr_string).release ();
}
break;
case EXPLICIT_LOCATION:
- EL_STRING (location)
- = explicit_location_to_string (EL_EXPLICIT (location)).release ();
+ location->as_string
+ = explicit_location_to_string (&location->u.explicit_loc).release ();
break;
case PROBE_LOCATION:
- EL_STRING (location) = xstrdup (EL_PROBE (location));
+ location->as_string = xstrdup (location->u.addr_string);
break;
default:
@@ -437,7 +434,7 @@ event_location_to_string (struct event_location *location)
}
}
- return EL_STRING (location);
+ return location->as_string;
}
/* Find an instance of the quote character C in the string S that is
@@ -804,17 +801,17 @@ string_to_explicit_location (const char **argp,
{
set_oarg (explicit_location_lex_one (argp, language,
completion_info));
- EL_EXPLICIT (location)->source_filename = oarg.release ();
+ location->u.explicit_loc.source_filename = oarg.release ();
}
else if (strncmp (opt.get (), "-function", len) == 0)
{
set_oarg (explicit_location_lex_one_function (argp, language,
completion_info));
- EL_EXPLICIT (location)->function_name = oarg.release ();
+ location->u.explicit_loc.function_name = oarg.release ();
}
else if (strncmp (opt.get (), "-qualified", len) == 0)
{
- EL_EXPLICIT (location)->func_name_match_type
+ location->u.explicit_loc.func_name_match_type
= symbol_name_match_type::FULL;
}
else if (strncmp (opt.get (), "-line", len) == 0)
@@ -823,7 +820,7 @@ string_to_explicit_location (const char **argp,
*argp = skip_spaces (*argp);
if (have_oarg)
{
- EL_EXPLICIT (location)->line_offset
+ location->u.explicit_loc.line_offset
= linespec_parse_line_offset (oarg.get ());
continue;
}
@@ -831,7 +828,7 @@ string_to_explicit_location (const char **argp,
else if (strncmp (opt.get (), "-label", len) == 0)
{
set_oarg (explicit_location_lex_one (argp, language, completion_info));
- EL_EXPLICIT (location)->label_name = oarg.release ();
+ location->u.explicit_loc.label_name = oarg.release ();
}
/* Only emit an "invalid argument" error for options
that look like option strings. */
@@ -861,10 +858,10 @@ string_to_explicit_location (const char **argp,
/* One special error check: If a source filename was given
without offset, function, or label, issue an error. */
- if (EL_EXPLICIT (location)->source_filename != NULL
- && EL_EXPLICIT (location)->function_name == NULL
- && EL_EXPLICIT (location)->label_name == NULL
- && (EL_EXPLICIT (location)->line_offset.sign == LINE_OFFSET_UNKNOWN)
+ if (location->u.explicit_loc.source_filename != NULL
+ && location->u.explicit_loc.function_name == NULL
+ && location->u.explicit_loc.label_name == NULL
+ && (location->u.explicit_loc.line_offset.sign == LINE_OFFSET_UNKNOWN)
&& completion_info == NULL)
{
error (_("Source filename requires function, label, or "
@@ -940,7 +937,7 @@ string_to_event_location (const char **stringp,
"-qualified", otherwise string_to_explicit_location would
have thrown an error. Save the flags for "basic" linespec
parsing below and discard the explicit location. */
- match_type = EL_EXPLICIT (location)->func_name_match_type;
+ match_type = location->u.explicit_loc.func_name_match_type;
}
/* Everything else is a "basic" linespec, address, or probe
@@ -953,7 +950,7 @@ string_to_event_location (const char **stringp,
int
event_location_empty_p (const struct event_location *location)
{
- switch (EL_TYPE (location))
+ switch (location->type)
{
case LINESPEC_LOCATION:
/* Linespecs are never "empty." (NULL is a valid linespec) */
@@ -963,14 +960,14 @@ event_location_empty_p (const struct event_location *location)
return 0;
case EXPLICIT_LOCATION:
- return (EL_EXPLICIT (location)->source_filename == NULL
- && EL_EXPLICIT (location)->function_name == NULL
- && EL_EXPLICIT (location)->label_name == NULL
- && (EL_EXPLICIT (location)->line_offset.sign
+ return (location->u.explicit_loc.source_filename == NULL
+ && location->u.explicit_loc.function_name == NULL
+ && location->u.explicit_loc.label_name == NULL
+ && (location->u.explicit_loc.line_offset.sign
== LINE_OFFSET_UNKNOWN));
case PROBE_LOCATION:
- return EL_PROBE (location) == NULL;
+ return location->u.addr_string == NULL;
default:
gdb_assert_not_reached ("unknown event location type");
@@ -983,6 +980,6 @@ void
set_event_location_string (struct event_location *location,
gdb::unique_xmalloc_ptr<char> string)
{
- xfree (EL_STRING (location));
- EL_STRING (location) = string.release ();
+ xfree (location->as_string);
+ location->as_string = string.release ();
}
--
2.31.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4/6] Split event_location into subclasses
2022-01-14 16:50 [PATCH 0/6] C++-ify location.c Tom Tromey
` (2 preceding siblings ...)
2022-01-14 16:50 ` [PATCH 3/6] Remove EL_* macros from location.c Tom Tromey
@ 2022-01-14 16:50 ` Tom Tromey
2022-01-18 10:33 ` Andrew Burgess
2022-01-14 16:50 ` [PATCH 5/6] Use std::string in event_location Tom Tromey
` (2 subsequent siblings)
6 siblings, 1 reply; 10+ messages in thread
From: Tom Tromey @ 2022-01-14 16:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
event_location uses the old C-style discriminated union approach.
However, it's better to use subclassing, as this makes the code
clearer and removes some chances for error. This also enables future
cleanups to avoid manual memory management and copies.
---
gdb/location.c | 515 +++++++++++++++++++++++++++----------------------
1 file changed, 279 insertions(+), 236 deletions(-)
diff --git a/gdb/location.c b/gdb/location.c
index 9c33ea4746e..4e5d043445c 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -29,33 +29,268 @@
#include <ctype.h>
#include <string.h>
+static gdb::unique_xmalloc_ptr<char> explicit_location_to_string
+ (const struct explicit_location *explicit_loc);
+
/* An event location used to set a stop event in the inferior.
This structure is an amalgam of the various ways
to specify where a stop event should be set. */
struct event_location
{
+ virtual ~event_location ()
+ {
+ xfree (as_string);
+ }
+
+ /* Clone this object. */
+ virtual event_location_up clone () const = 0;
+
+ /* Return true if this location is empty, false otherwise. */
+ virtual bool empty_p () const = 0;
+
+ /* Return a string representation of this location. */
+ const char *to_string ()
+ {
+ if (as_string == nullptr)
+ as_string = compute_string ();
+ return as_string;
+ }
+
+ DISABLE_COPY_AND_ASSIGN (event_location);
+
/* The type of this breakpoint specification. */
enum event_location_type type;
- union
+ /* Cached string representation of this location. This is used, e.g., to
+ save stop event locations to file. Malloc'd. */
+ char *as_string = nullptr;
+
+protected:
+
+ explicit event_location (enum event_location_type t)
+ : type (t)
{
- /* A probe. */
- char *addr_string;
+ }
- /* A "normal" linespec. */
- struct linespec_location linespec_location;
+ explicit event_location (const event_location *to_clone)
+ : type (to_clone->type),
+ as_string (to_clone->as_string == nullptr
+ ? nullptr
+ : xstrdup (to_clone->as_string))
+ {
+ }
- /* An address in the inferior. */
- CORE_ADDR address;
+ /* Compute the string representation of this object. This is called
+ by to_string when needed. */
+ virtual char *compute_string () = 0;
+};
- /* An explicit location. */
- struct explicit_location explicit_loc;
- } u;
+/* A probe. */
+struct event_location_probe : public event_location
+{
+ explicit event_location_probe (const char *probe)
+ : event_location (PROBE_LOCATION),
+ addr_string (probe == nullptr
+ ? nullptr
+ : xstrdup (probe))
+ {
+ }
- /* Cached string representation of this location. This is used, e.g., to
- save stop event locations to file. Malloc'd. */
- char *as_string;
+ ~event_location_probe ()
+ {
+ xfree (addr_string);
+ }
+
+ event_location_up clone () const override
+ {
+ return event_location_up (new event_location_probe (this));
+ }
+
+ bool empty_p () const override
+ {
+ return addr_string == nullptr;
+ }
+
+ char *addr_string;
+
+protected:
+
+ explicit event_location_probe (const event_location_probe *to_clone)
+ : event_location (to_clone),
+ addr_string (to_clone->addr_string == nullptr
+ ? nullptr
+ : xstrdup (to_clone->addr_string))
+ {
+ }
+
+ char *compute_string () override
+ {
+ return xstrdup (addr_string);
+ }
+};
+
+/* A "normal" linespec. */
+struct event_location_linespec : public event_location
+{
+ event_location_linespec (const char **linespec,
+ symbol_name_match_type match_type)
+ : event_location (LINESPEC_LOCATION)
+ {
+ linespec_location.match_type = match_type;
+ if (*linespec != NULL)
+ {
+ const char *p;
+ const char *orig = *linespec;
+
+ linespec_lex_to_end (linespec);
+ p = remove_trailing_whitespace (orig, *linespec);
+ if ((p - orig) > 0)
+ linespec_location.spec_string = savestring (orig, p - orig);
+ }
+ }
+
+ ~event_location_linespec ()
+ {
+ xfree (linespec_location.spec_string);
+ }
+
+ event_location_up clone () const override
+ {
+ return event_location_up (new event_location_linespec (this));
+ }
+
+ bool empty_p () const override
+ {
+ return false;
+ }
+
+ struct linespec_location linespec_location {};
+
+protected:
+
+ explicit event_location_linespec (const event_location_linespec *to_clone)
+ : event_location (to_clone),
+ linespec_location (to_clone->linespec_location)
+ {
+ if (linespec_location.spec_string != nullptr)
+ linespec_location.spec_string = xstrdup (linespec_location.spec_string);
+ }
+
+ char *compute_string () override
+ {
+ if (linespec_location.spec_string != nullptr)
+ {
+ struct linespec_location *ls = &linespec_location;
+ if (ls->match_type == symbol_name_match_type::FULL)
+ return concat ("-qualified ", ls->spec_string, (char *) nullptr);
+ else
+ return xstrdup (ls->spec_string);
+ }
+ return nullptr;
+ }
+};
+
+/* An address in the inferior. */
+struct event_location_address : public event_location
+{
+ event_location_address (CORE_ADDR addr, const char *addr_string,
+ int addr_string_len)
+ : event_location (ADDRESS_LOCATION),
+ address (addr)
+ {
+ if (addr_string != nullptr)
+ as_string = xstrndup (addr_string, addr_string_len);
+ }
+
+ event_location_up clone () const override
+ {
+ return event_location_up (new event_location_address (this));
+ }
+
+ bool empty_p () const override
+ {
+ return false;
+ }
+
+ CORE_ADDR address;
+
+protected:
+
+ event_location_address (const event_location_address *to_clone)
+ : event_location (to_clone),
+ address (to_clone->address)
+ {
+ }
+
+ char *compute_string () override
+ {
+ const char *addr_string = core_addr_to_string (address);
+ return xstrprintf ("*%s", addr_string).release ();
+ }
+};
+
+/* An explicit location. */
+struct event_location_explicit : public event_location
+{
+ explicit event_location_explicit (const struct explicit_location *loc)
+ : event_location (EXPLICIT_LOCATION)
+ {
+ copy_loc (loc);
+ }
+
+ ~event_location_explicit ()
+ {
+ xfree (explicit_loc.source_filename);
+ xfree (explicit_loc.function_name);
+ xfree (explicit_loc.label_name);
+ }
+
+ event_location_up clone () const override
+ {
+ return event_location_up (new event_location_explicit (this));
+ }
+
+ bool empty_p () const override
+ {
+ return (explicit_loc.source_filename == nullptr
+ && explicit_loc.function_name == nullptr
+ && explicit_loc.label_name == nullptr
+ && explicit_loc.line_offset.sign == LINE_OFFSET_UNKNOWN);
+ }
+
+ struct explicit_location explicit_loc;
+
+protected:
+
+ explicit event_location_explicit (const event_location_explicit *to_clone)
+ : event_location (to_clone)
+ {
+ copy_loc (&to_clone->explicit_loc);
+ }
+
+ char *compute_string () override
+ {
+ return explicit_location_to_string (&explicit_loc).release ();
+ }
+
+private:
+
+ void copy_loc (const struct explicit_location *loc)
+ {
+ initialize_explicit_location (&explicit_loc);
+ if (loc != nullptr)
+ {
+ explicit_loc.func_name_match_type = loc->func_name_match_type;
+ if (loc->source_filename != nullptr)
+ explicit_loc.source_filename = xstrdup (loc->source_filename);
+ if (loc->function_name != nullptr)
+ explicit_loc.function_name = xstrdup (loc->function_name);
+ if (loc->label_name != nullptr)
+ explicit_loc.label_name = xstrdup (loc->label_name);
+ explicit_loc.line_offset = loc->line_offset;
+ }
+ }
};
/* See description in location.h. */
@@ -82,23 +317,8 @@ event_location_up
new_linespec_location (const char **linespec,
symbol_name_match_type match_type)
{
- struct event_location *location;
-
- location = XCNEW (struct event_location);
- location->type = LINESPEC_LOCATION;
- location->u.linespec_location.match_type = match_type;
- if (*linespec != NULL)
- {
- const char *p;
- const char *orig = *linespec;
-
- linespec_lex_to_end (linespec);
- p = remove_trailing_whitespace (orig, *linespec);
- if ((p - orig) > 0)
- location->u.linespec_location.spec_string
- = savestring (orig, p - orig);
- }
- return event_location_up (location);
+ return event_location_up (new event_location_linespec (linespec,
+ match_type));
}
/* See description in location.h. */
@@ -107,7 +327,7 @@ const linespec_location *
get_linespec_location (const struct event_location *location)
{
gdb_assert (location->type == LINESPEC_LOCATION);
- return &location->u.linespec_location;
+ return &((event_location_linespec *) location)->linespec_location;
}
/* See description in location.h. */
@@ -116,14 +336,8 @@ event_location_up
new_address_location (CORE_ADDR addr, const char *addr_string,
int addr_string_len)
{
- struct event_location *location;
-
- location = XCNEW (struct event_location);
- location->type = ADDRESS_LOCATION;
- location->u.address = addr;
- if (addr_string != NULL)
- location->as_string = xstrndup (addr_string, addr_string_len);
- return event_location_up (location);
+ return event_location_up (new event_location_address (addr, addr_string,
+ addr_string_len));
}
/* See description in location.h. */
@@ -132,7 +346,7 @@ CORE_ADDR
get_address_location (const struct event_location *location)
{
gdb_assert (location->type == ADDRESS_LOCATION);
- return location->u.address;
+ return ((event_location_address *) location)->address;
}
/* See description in location.h. */
@@ -149,13 +363,7 @@ get_address_string_location (const struct event_location *location)
event_location_up
new_probe_location (const char *probe)
{
- struct event_location *location;
-
- location = XCNEW (struct event_location);
- location->type = PROBE_LOCATION;
- if (probe != NULL)
- location->u.addr_string = xstrdup (probe);
- return event_location_up (location);
+ return event_location_up (new event_location_probe (probe));
}
/* See description in location.h. */
@@ -164,7 +372,7 @@ const char *
get_probe_location (const struct event_location *location)
{
gdb_assert (location->type == PROBE_LOCATION);
- return location->u.addr_string;
+ return ((event_location_probe *) location)->addr_string;
}
/* See description in location.h. */
@@ -172,34 +380,7 @@ get_probe_location (const struct event_location *location)
event_location_up
new_explicit_location (const struct explicit_location *explicit_loc)
{
- struct event_location tmp;
-
- memset (&tmp, 0, sizeof (struct event_location));
- tmp.type = EXPLICIT_LOCATION;
- initialize_explicit_location (&tmp.u.explicit_loc);
- if (explicit_loc != NULL)
- {
- tmp.u.explicit_loc.func_name_match_type
- = explicit_loc->func_name_match_type;
-
- if (explicit_loc->source_filename != NULL)
- {
- tmp.u.explicit_loc.source_filename
- = explicit_loc->source_filename;
- }
-
- if (explicit_loc->function_name != NULL)
- tmp.u.explicit_loc.function_name
- = explicit_loc->function_name;
-
- if (explicit_loc->label_name != NULL)
- tmp.u.explicit_loc.label_name = explicit_loc->label_name;
-
- if (explicit_loc->line_offset.sign != LINE_OFFSET_UNKNOWN)
- tmp.u.explicit_loc.line_offset = explicit_loc->line_offset;
- }
-
- return copy_event_location (&tmp);
+ return event_location_up (new event_location_explicit (explicit_loc));
}
/* See description in location.h. */
@@ -208,7 +389,7 @@ struct explicit_location *
get_explicit_location (struct event_location *location)
{
gdb_assert (location->type == EXPLICIT_LOCATION);
- return &location->u.explicit_loc;
+ return &((event_location_explicit *) location)->explicit_loc;
}
/* See description in location.h. */
@@ -217,7 +398,7 @@ const struct explicit_location *
get_explicit_location_const (const struct event_location *location)
{
gdb_assert (location->type == EXPLICIT_LOCATION);
- return &location->u.explicit_loc;
+ return &((event_location_explicit *) location)->explicit_loc;
}
/* This convenience function returns a malloc'd string which
@@ -301,91 +482,13 @@ explicit_location_to_linespec (const struct explicit_location *explicit_loc)
event_location_up
copy_event_location (const struct event_location *src)
{
- struct event_location *dst;
-
- dst = XCNEW (struct event_location);
- dst->type = src->type;
- if (src->as_string != NULL)
- dst->as_string = xstrdup (src->as_string);
-
- switch (src->type)
- {
- case LINESPEC_LOCATION:
- dst->u.linespec_location.match_type
- = src->u.linespec_location.match_type;
- if (src->u.linespec_location.spec_string != NULL)
- dst->u.linespec_location.spec_string
- = xstrdup (src->u.linespec_location.spec_string);
- break;
-
- case ADDRESS_LOCATION:
- dst->u.address = src->u.address;
- break;
-
- case EXPLICIT_LOCATION:
- dst->u.explicit_loc.func_name_match_type
- = src->u.explicit_loc.func_name_match_type;
- if (src->u.explicit_loc.source_filename != NULL)
- dst->u.explicit_loc.source_filename
- = xstrdup (src->u.explicit_loc.source_filename);
-
- if (src->u.explicit_loc.function_name != NULL)
- dst->u.explicit_loc.function_name
- = xstrdup (src->u.explicit_loc.function_name);
-
- if (src->u.explicit_loc.label_name != NULL)
- dst->u.explicit_loc.label_name
- = xstrdup (src->u.explicit_loc.label_name);
-
- dst->u.explicit_loc.line_offset = src->u.explicit_loc.line_offset;
- break;
-
-
- case PROBE_LOCATION:
- if (src->u.addr_string != NULL)
- dst->u.addr_string = xstrdup (src->u.addr_string);
- break;
-
- default:
- gdb_assert_not_reached ("unknown event location type");
- }
-
- return event_location_up (dst);
+ return src->clone ();
}
void
event_location_deleter::operator() (event_location *location) const
{
- if (location != NULL)
- {
- xfree (location->as_string);
-
- switch (location->type)
- {
- case LINESPEC_LOCATION:
- xfree (location->u.linespec_location.spec_string);
- break;
-
- case ADDRESS_LOCATION:
- /* Nothing to do. */
- break;
-
- case EXPLICIT_LOCATION:
- xfree (location->u.explicit_loc.source_filename);
- xfree (location->u.explicit_loc.function_name);
- xfree (location->u.explicit_loc.label_name);
- break;
-
- case PROBE_LOCATION:
- xfree (location->u.addr_string);
- break;
-
- default:
- gdb_assert_not_reached ("unknown event location type");
- }
-
- xfree (location);
- }
+ delete location;
}
/* See description in location.h. */
@@ -393,48 +496,7 @@ event_location_deleter::operator() (event_location *location) const
const char *
event_location_to_string (struct event_location *location)
{
- if (location->as_string == NULL)
- {
- switch (location->type)
- {
- case LINESPEC_LOCATION:
- if (location->u.linespec_location.spec_string != NULL)
- {
- linespec_location *ls = &location->u.linespec_location;
- if (ls->match_type == symbol_name_match_type::FULL)
- {
- location->as_string
- = concat ("-qualified ", ls->spec_string, (char *) NULL);
- }
- else
- location->as_string = xstrdup (ls->spec_string);
- }
- break;
-
- case ADDRESS_LOCATION:
- {
- const char *addr_string
- = core_addr_to_string (location->u.address);
- location->as_string
- = xstrprintf ("*%s", addr_string).release ();
- }
- break;
-
- case EXPLICIT_LOCATION:
- location->as_string
- = explicit_location_to_string (&location->u.explicit_loc).release ();
- break;
-
- case PROBE_LOCATION:
- location->as_string = xstrdup (location->u.addr_string);
- break;
-
- default:
- gdb_assert_not_reached ("unknown event location type");
- }
- }
-
- return location->as_string;
+ return location->to_string ();
}
/* Find an instance of the quote character C in the string S that is
@@ -720,8 +782,6 @@ string_to_explicit_location (const char **argp,
const struct language_defn *language,
explicit_completion_info *completion_info)
{
- event_location_up location;
-
/* It is assumed that input beginning with '-' and a non-digit
character is an explicit location. "-p" is reserved, though,
for probe locations. */
@@ -732,7 +792,8 @@ string_to_explicit_location (const char **argp,
|| ((*argp)[0] == '-' && (*argp)[1] == 'p'))
return NULL;
- location = new_explicit_location (NULL);
+ std::unique_ptr<event_location_explicit> location
+ (new event_location_explicit ((const explicit_location *) nullptr));
/* Process option/argument pairs. dprintf_command
requires that processing stop on ','. */
@@ -801,17 +862,17 @@ string_to_explicit_location (const char **argp,
{
set_oarg (explicit_location_lex_one (argp, language,
completion_info));
- location->u.explicit_loc.source_filename = oarg.release ();
+ location->explicit_loc.source_filename = oarg.release ();
}
else if (strncmp (opt.get (), "-function", len) == 0)
{
set_oarg (explicit_location_lex_one_function (argp, language,
completion_info));
- location->u.explicit_loc.function_name = oarg.release ();
+ location->explicit_loc.function_name = oarg.release ();
}
else if (strncmp (opt.get (), "-qualified", len) == 0)
{
- location->u.explicit_loc.func_name_match_type
+ location->explicit_loc.func_name_match_type
= symbol_name_match_type::FULL;
}
else if (strncmp (opt.get (), "-line", len) == 0)
@@ -820,7 +881,7 @@ string_to_explicit_location (const char **argp,
*argp = skip_spaces (*argp);
if (have_oarg)
{
- location->u.explicit_loc.line_offset
+ location->explicit_loc.line_offset
= linespec_parse_line_offset (oarg.get ());
continue;
}
@@ -828,7 +889,7 @@ string_to_explicit_location (const char **argp,
else if (strncmp (opt.get (), "-label", len) == 0)
{
set_oarg (explicit_location_lex_one (argp, language, completion_info));
- location->u.explicit_loc.label_name = oarg.release ();
+ location->explicit_loc.label_name = oarg.release ();
}
/* Only emit an "invalid argument" error for options
that look like option strings. */
@@ -858,17 +919,17 @@ string_to_explicit_location (const char **argp,
/* One special error check: If a source filename was given
without offset, function, or label, issue an error. */
- if (location->u.explicit_loc.source_filename != NULL
- && location->u.explicit_loc.function_name == NULL
- && location->u.explicit_loc.label_name == NULL
- && (location->u.explicit_loc.line_offset.sign == LINE_OFFSET_UNKNOWN)
+ if (location->explicit_loc.source_filename != NULL
+ && location->explicit_loc.function_name == NULL
+ && location->explicit_loc.label_name == NULL
+ && (location->explicit_loc.line_offset.sign == LINE_OFFSET_UNKNOWN)
&& completion_info == NULL)
{
error (_("Source filename requires function, label, or "
"line offset."));
}
- return location;
+ return event_location_up (location.release ());
}
/* See description in location.h. */
@@ -937,7 +998,10 @@ string_to_event_location (const char **stringp,
"-qualified", otherwise string_to_explicit_location would
have thrown an error. Save the flags for "basic" linespec
parsing below and discard the explicit location. */
- match_type = location->u.explicit_loc.func_name_match_type;
+ event_location_explicit *xloc
+ = dynamic_cast<event_location_explicit *> (location.get ());
+ gdb_assert (xloc != nullptr);
+ match_type = xloc->explicit_loc.func_name_match_type;
}
/* Everything else is a "basic" linespec, address, or probe
@@ -950,28 +1014,7 @@ string_to_event_location (const char **stringp,
int
event_location_empty_p (const struct event_location *location)
{
- switch (location->type)
- {
- case LINESPEC_LOCATION:
- /* Linespecs are never "empty." (NULL is a valid linespec) */
- return 0;
-
- case ADDRESS_LOCATION:
- return 0;
-
- case EXPLICIT_LOCATION:
- return (location->u.explicit_loc.source_filename == NULL
- && location->u.explicit_loc.function_name == NULL
- && location->u.explicit_loc.label_name == NULL
- && (location->u.explicit_loc.line_offset.sign
- == LINE_OFFSET_UNKNOWN));
-
- case PROBE_LOCATION:
- return location->u.addr_string == NULL;
-
- default:
- gdb_assert_not_reached ("unknown event location type");
- }
+ return location->empty_p ();
}
/* See description in location.h. */
--
2.31.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 5/6] Use std::string in event_location
2022-01-14 16:50 [PATCH 0/6] C++-ify location.c Tom Tromey
` (3 preceding siblings ...)
2022-01-14 16:50 ` [PATCH 4/6] Split event_location into subclasses Tom Tromey
@ 2022-01-14 16:50 ` Tom Tromey
2022-01-14 16:50 ` [PATCH 6/6] Simplify event_location_probe Tom Tromey
2022-01-18 10:36 ` [PATCH 0/6] C++-ify location.c Andrew Burgess
6 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2022-01-14 16:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This changes event_location to use std::string, removing some manual
memory management, and an unnecessary string copy.
---
gdb/location.c | 66 ++++++++++++++++++++++++--------------------------
gdb/location.h | 7 +++---
2 files changed, 34 insertions(+), 39 deletions(-)
diff --git a/gdb/location.c b/gdb/location.c
index 4e5d043445c..68099e39041 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -29,7 +29,7 @@
#include <ctype.h>
#include <string.h>
-static gdb::unique_xmalloc_ptr<char> explicit_location_to_string
+static std::string explicit_location_to_string
(const struct explicit_location *explicit_loc);
/* An event location used to set a stop event in the inferior.
@@ -38,10 +38,7 @@ static gdb::unique_xmalloc_ptr<char> explicit_location_to_string
struct event_location
{
- virtual ~event_location ()
- {
- xfree (as_string);
- }
+ virtual ~event_location () = default;
/* Clone this object. */
virtual event_location_up clone () const = 0;
@@ -50,11 +47,13 @@ struct event_location
virtual bool empty_p () const = 0;
/* Return a string representation of this location. */
- const char *to_string ()
+ const char *to_string () const
{
- if (as_string == nullptr)
+ if (as_string.empty ())
as_string = compute_string ();
- return as_string;
+ if (as_string.empty ())
+ return nullptr;
+ return as_string.c_str ();
}
DISABLE_COPY_AND_ASSIGN (event_location);
@@ -62,9 +61,9 @@ struct event_location
/* The type of this breakpoint specification. */
enum event_location_type type;
- /* Cached string representation of this location. This is used, e.g., to
- save stop event locations to file. Malloc'd. */
- char *as_string = nullptr;
+ /* Cached string representation of this location. This is used,
+ e.g., to save stop event locations to file. */
+ mutable std::string as_string;
protected:
@@ -75,15 +74,13 @@ struct event_location
explicit event_location (const event_location *to_clone)
: type (to_clone->type),
- as_string (to_clone->as_string == nullptr
- ? nullptr
- : xstrdup (to_clone->as_string))
+ as_string (to_clone->as_string)
{
}
/* Compute the string representation of this object. This is called
by to_string when needed. */
- virtual char *compute_string () = 0;
+ virtual std::string compute_string () const = 0;
};
/* A probe. */
@@ -124,9 +121,9 @@ struct event_location_probe : public event_location
{
}
- char *compute_string () override
+ std::string compute_string () const override
{
- return xstrdup (addr_string);
+ return addr_string;
}
};
@@ -177,17 +174,17 @@ struct event_location_linespec : public event_location
linespec_location.spec_string = xstrdup (linespec_location.spec_string);
}
- char *compute_string () override
+ std::string compute_string () const override
{
if (linespec_location.spec_string != nullptr)
{
- struct linespec_location *ls = &linespec_location;
+ const struct linespec_location *ls = &linespec_location;
if (ls->match_type == symbol_name_match_type::FULL)
- return concat ("-qualified ", ls->spec_string, (char *) nullptr);
+ return std::string ("-qualified ") + ls->spec_string;
else
- return xstrdup (ls->spec_string);
+ return ls->spec_string;
}
- return nullptr;
+ return {};
}
};
@@ -200,7 +197,7 @@ struct event_location_address : public event_location
address (addr)
{
if (addr_string != nullptr)
- as_string = xstrndup (addr_string, addr_string_len);
+ as_string = std::string (addr_string, addr_string_len);
}
event_location_up clone () const override
@@ -223,10 +220,10 @@ struct event_location_address : public event_location
{
}
- char *compute_string () override
+ std::string compute_string () const override
{
const char *addr_string = core_addr_to_string (address);
- return xstrprintf ("*%s", addr_string).release ();
+ return std::string ("*") + addr_string;
}
};
@@ -269,9 +266,9 @@ struct event_location_explicit : public event_location
copy_loc (&to_clone->explicit_loc);
}
- char *compute_string () override
+ std::string compute_string () const override
{
- return explicit_location_to_string (&explicit_loc).release ();
+ return explicit_location_to_string (&explicit_loc);
}
private:
@@ -355,7 +352,7 @@ const char *
get_address_string_location (const struct event_location *location)
{
gdb_assert (location->type == ADDRESS_LOCATION);
- return location->as_string;
+ return location->to_string ();
}
/* See description in location.h. */
@@ -407,7 +404,7 @@ get_explicit_location_const (const struct event_location *location)
AS_LINESPEC is true if this string should be a linespec.
Otherwise it will be output in explicit form. */
-static gdb::unique_xmalloc_ptr<char>
+static std::string
explicit_to_string_internal (bool as_linespec,
const struct explicit_location *explicit_loc)
{
@@ -458,12 +455,12 @@ explicit_to_string_internal (bool as_linespec,
explicit_loc->line_offset.offset);
}
- return make_unique_xstrdup (buf.c_str ());
+ return std::move (buf.string ());
}
/* See description in location.h. */
-static gdb::unique_xmalloc_ptr<char>
+static std::string
explicit_location_to_string (const struct explicit_location *explicit_loc)
{
return explicit_to_string_internal (false, explicit_loc);
@@ -471,7 +468,7 @@ explicit_location_to_string (const struct explicit_location *explicit_loc)
/* See description in location.h. */
-gdb::unique_xmalloc_ptr<char>
+std::string
explicit_location_to_linespec (const struct explicit_location *explicit_loc)
{
return explicit_to_string_internal (true, explicit_loc);
@@ -1021,8 +1018,7 @@ event_location_empty_p (const struct event_location *location)
void
set_event_location_string (struct event_location *location,
- gdb::unique_xmalloc_ptr<char> string)
+ std::string &&string)
{
- xfree (location->as_string);
- location->as_string = string.release ();
+ location->as_string = std::move (string);
}
diff --git a/gdb/location.h b/gdb/location.h
index b391ce3b1dd..4cabf261383 100644
--- a/gdb/location.h
+++ b/gdb/location.h
@@ -112,7 +112,7 @@ extern enum event_location_type
/* Return a linespec string representation of the given explicit
location. The location must already be canonicalized/valid. */
-extern gdb::unique_xmalloc_ptr<char>
+extern std::string
explicit_location_to_linespec (const struct explicit_location *explicit_loc);
/* Return a string representation of the LOCATION.
@@ -275,11 +275,10 @@ extern event_location_up
extern int event_location_empty_p (const struct event_location *location);
-/* Set the location's string representation. If STRING is NULL, clear
- the string representation. */
+/* Set the location's string representation. */
extern void
set_event_location_string (struct event_location *location,
- gdb::unique_xmalloc_ptr<char> string);
+ std::string &&string);
#endif /* LOCATION_H */
--
2.31.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 6/6] Simplify event_location_probe
2022-01-14 16:50 [PATCH 0/6] C++-ify location.c Tom Tromey
` (4 preceding siblings ...)
2022-01-14 16:50 ` [PATCH 5/6] Use std::string in event_location Tom Tromey
@ 2022-01-14 16:50 ` Tom Tromey
2022-01-18 10:36 ` [PATCH 0/6] C++-ify location.c Andrew Burgess
6 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2022-01-14 16:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
event_location_probe currently stores two strings, but really only
needs one. This patch simplifies it and removes some unnecessary
copies as well.
---
gdb/location.c | 35 ++++++++++++++---------------------
gdb/location.h | 2 +-
gdb/probe.c | 2 +-
3 files changed, 16 insertions(+), 23 deletions(-)
diff --git a/gdb/location.c b/gdb/location.c
index 68099e39041..c31d1225e57 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -72,6 +72,12 @@ struct event_location
{
}
+ event_location (enum event_location_type t, std::string &&str)
+ : type (t),
+ as_string (std::move (str))
+ {
+ }
+
explicit event_location (const event_location *to_clone)
: type (to_clone->type),
as_string (to_clone->as_string)
@@ -86,17 +92,9 @@ struct event_location
/* A probe. */
struct event_location_probe : public event_location
{
- explicit event_location_probe (const char *probe)
- : event_location (PROBE_LOCATION),
- addr_string (probe == nullptr
- ? nullptr
- : xstrdup (probe))
- {
- }
-
- ~event_location_probe ()
+ explicit event_location_probe (std::string &&probe)
+ : event_location (PROBE_LOCATION, std::move (probe))
{
- xfree (addr_string);
}
event_location_up clone () const override
@@ -106,24 +104,19 @@ struct event_location_probe : public event_location
bool empty_p () const override
{
- return addr_string == nullptr;
+ return false;
}
- char *addr_string;
-
protected:
explicit event_location_probe (const event_location_probe *to_clone)
- : event_location (to_clone),
- addr_string (to_clone->addr_string == nullptr
- ? nullptr
- : xstrdup (to_clone->addr_string))
+ : event_location (to_clone)
{
}
std::string compute_string () const override
{
- return addr_string;
+ return std::move (as_string);
}
};
@@ -358,9 +351,9 @@ get_address_string_location (const struct event_location *location)
/* See description in location.h. */
event_location_up
-new_probe_location (const char *probe)
+new_probe_location (std::string &&probe)
{
- return event_location_up (new event_location_probe (probe));
+ return event_location_up (new event_location_probe (std::move (probe)));
}
/* See description in location.h. */
@@ -369,7 +362,7 @@ const char *
get_probe_location (const struct event_location *location)
{
gdb_assert (location->type == PROBE_LOCATION);
- return ((event_location_probe *) location)->addr_string;
+ return location->to_string ();
}
/* See description in location.h. */
diff --git a/gdb/location.h b/gdb/location.h
index 4cabf261383..848f6458e5e 100644
--- a/gdb/location.h
+++ b/gdb/location.h
@@ -169,7 +169,7 @@ extern const char *
/* Create a new probe location. */
-extern event_location_up new_probe_location (const char *probe);
+extern event_location_up new_probe_location (std::string &&probe);
/* Return the probe location (a string) of the given event_location
(which must be of type PROBE_LOCATION). */
diff --git a/gdb/probe.c b/gdb/probe.c
index b8da177a0ae..689b5f022e4 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -204,7 +204,7 @@ parse_probes (const struct event_location *location,
std::string canon (arg_start, arg_end - arg_start);
canonical->special_display = 1;
canonical->pre_expanded = 1;
- canonical->location = new_probe_location (canon.c_str ());
+ canonical->location = new_probe_location (std::move (canon));
}
return result;
--
2.31.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/6] Split event_location into subclasses
2022-01-14 16:50 ` [PATCH 4/6] Split event_location into subclasses Tom Tromey
@ 2022-01-18 10:33 ` Andrew Burgess
2022-01-18 16:59 ` Tom Tromey
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Burgess @ 2022-01-18 10:33 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
* Tom Tromey <tom@tromey.com> [2022-01-14 09:50:06 -0700]:
> event_location uses the old C-style discriminated union approach.
> However, it's better to use subclassing, as this makes the code
> clearer and removes some chances for error. This also enables future
> cleanups to avoid manual memory management and copies.
> ---
> gdb/location.c | 515 +++++++++++++++++++++++++++----------------------
> 1 file changed, 279 insertions(+), 236 deletions(-)
>
> diff --git a/gdb/location.c b/gdb/location.c
> index 9c33ea4746e..4e5d043445c 100644
> --- a/gdb/location.c
> +++ b/gdb/location.c
> @@ -29,33 +29,268 @@
> #include <ctype.h>
> #include <string.h>
>
> +static gdb::unique_xmalloc_ptr<char> explicit_location_to_string
> + (const struct explicit_location *explicit_loc);
> +
> /* An event location used to set a stop event in the inferior.
> This structure is an amalgam of the various ways
> to specify where a stop event should be set. */
I think this comment is now out of date.
>
> struct event_location
> {
> + virtual ~event_location ()
> + {
> + xfree (as_string);
> + }
> +
> + /* Clone this object. */
> + virtual event_location_up clone () const = 0;
> +
> + /* Return true if this location is empty, false otherwise. */
> + virtual bool empty_p () const = 0;
> +
> + /* Return a string representation of this location. */
> + const char *to_string ()
> + {
> + if (as_string == nullptr)
> + as_string = compute_string ();
> + return as_string;
> + }
> +
> + DISABLE_COPY_AND_ASSIGN (event_location);
> +
> /* The type of this breakpoint specification. */
> enum event_location_type type;
>
> - union
> + /* Cached string representation of this location. This is used, e.g., to
> + save stop event locations to file. Malloc'd. */
> + char *as_string = nullptr;
I wonder if you considered making these private, at least for type,
and adding a read accessor? For as_string making it protected would
seem like a better choice, we still need write access from
set_event_location_string, but that itself could become a member
function, and get_address_string_location would currently seem to need
an accessor, though I notice in a later patch you change
get_address_string_location to call to_string.
If you think these changes would be better done later then this LGTM.
Thanks,
Andrew
> +
> +protected:
> +
> + explicit event_location (enum event_location_type t)
> + : type (t)
> {
> - /* A probe. */
> - char *addr_string;
> + }
>
> - /* A "normal" linespec. */
> - struct linespec_location linespec_location;
> + explicit event_location (const event_location *to_clone)
> + : type (to_clone->type),
> + as_string (to_clone->as_string == nullptr
> + ? nullptr
> + : xstrdup (to_clone->as_string))
> + {
> + }
>
> - /* An address in the inferior. */
> - CORE_ADDR address;
> + /* Compute the string representation of this object. This is called
> + by to_string when needed. */
> + virtual char *compute_string () = 0;
> +};
>
> - /* An explicit location. */
> - struct explicit_location explicit_loc;
> - } u;
> +/* A probe. */
> +struct event_location_probe : public event_location
> +{
> + explicit event_location_probe (const char *probe)
> + : event_location (PROBE_LOCATION),
> + addr_string (probe == nullptr
> + ? nullptr
> + : xstrdup (probe))
> + {
> + }
>
> - /* Cached string representation of this location. This is used, e.g., to
> - save stop event locations to file. Malloc'd. */
> - char *as_string;
> + ~event_location_probe ()
> + {
> + xfree (addr_string);
> + }
> +
> + event_location_up clone () const override
> + {
> + return event_location_up (new event_location_probe (this));
> + }
> +
> + bool empty_p () const override
> + {
> + return addr_string == nullptr;
> + }
> +
> + char *addr_string;
> +
> +protected:
> +
> + explicit event_location_probe (const event_location_probe *to_clone)
> + : event_location (to_clone),
> + addr_string (to_clone->addr_string == nullptr
> + ? nullptr
> + : xstrdup (to_clone->addr_string))
> + {
> + }
> +
> + char *compute_string () override
> + {
> + return xstrdup (addr_string);
> + }
> +};
> +
> +/* A "normal" linespec. */
> +struct event_location_linespec : public event_location
> +{
> + event_location_linespec (const char **linespec,
> + symbol_name_match_type match_type)
> + : event_location (LINESPEC_LOCATION)
> + {
> + linespec_location.match_type = match_type;
> + if (*linespec != NULL)
> + {
> + const char *p;
> + const char *orig = *linespec;
> +
> + linespec_lex_to_end (linespec);
> + p = remove_trailing_whitespace (orig, *linespec);
> + if ((p - orig) > 0)
> + linespec_location.spec_string = savestring (orig, p - orig);
> + }
> + }
> +
> + ~event_location_linespec ()
> + {
> + xfree (linespec_location.spec_string);
> + }
> +
> + event_location_up clone () const override
> + {
> + return event_location_up (new event_location_linespec (this));
> + }
> +
> + bool empty_p () const override
> + {
> + return false;
> + }
> +
> + struct linespec_location linespec_location {};
> +
> +protected:
> +
> + explicit event_location_linespec (const event_location_linespec *to_clone)
> + : event_location (to_clone),
> + linespec_location (to_clone->linespec_location)
> + {
> + if (linespec_location.spec_string != nullptr)
> + linespec_location.spec_string = xstrdup (linespec_location.spec_string);
> + }
> +
> + char *compute_string () override
> + {
> + if (linespec_location.spec_string != nullptr)
> + {
> + struct linespec_location *ls = &linespec_location;
> + if (ls->match_type == symbol_name_match_type::FULL)
> + return concat ("-qualified ", ls->spec_string, (char *) nullptr);
> + else
> + return xstrdup (ls->spec_string);
> + }
> + return nullptr;
> + }
> +};
> +
> +/* An address in the inferior. */
> +struct event_location_address : public event_location
> +{
> + event_location_address (CORE_ADDR addr, const char *addr_string,
> + int addr_string_len)
> + : event_location (ADDRESS_LOCATION),
> + address (addr)
> + {
> + if (addr_string != nullptr)
> + as_string = xstrndup (addr_string, addr_string_len);
> + }
> +
> + event_location_up clone () const override
> + {
> + return event_location_up (new event_location_address (this));
> + }
> +
> + bool empty_p () const override
> + {
> + return false;
> + }
> +
> + CORE_ADDR address;
> +
> +protected:
> +
> + event_location_address (const event_location_address *to_clone)
> + : event_location (to_clone),
> + address (to_clone->address)
> + {
> + }
> +
> + char *compute_string () override
> + {
> + const char *addr_string = core_addr_to_string (address);
> + return xstrprintf ("*%s", addr_string).release ();
> + }
> +};
> +
> +/* An explicit location. */
> +struct event_location_explicit : public event_location
> +{
> + explicit event_location_explicit (const struct explicit_location *loc)
> + : event_location (EXPLICIT_LOCATION)
> + {
> + copy_loc (loc);
> + }
> +
> + ~event_location_explicit ()
> + {
> + xfree (explicit_loc.source_filename);
> + xfree (explicit_loc.function_name);
> + xfree (explicit_loc.label_name);
> + }
> +
> + event_location_up clone () const override
> + {
> + return event_location_up (new event_location_explicit (this));
> + }
> +
> + bool empty_p () const override
> + {
> + return (explicit_loc.source_filename == nullptr
> + && explicit_loc.function_name == nullptr
> + && explicit_loc.label_name == nullptr
> + && explicit_loc.line_offset.sign == LINE_OFFSET_UNKNOWN);
> + }
> +
> + struct explicit_location explicit_loc;
> +
> +protected:
> +
> + explicit event_location_explicit (const event_location_explicit *to_clone)
> + : event_location (to_clone)
> + {
> + copy_loc (&to_clone->explicit_loc);
> + }
> +
> + char *compute_string () override
> + {
> + return explicit_location_to_string (&explicit_loc).release ();
> + }
> +
> +private:
> +
> + void copy_loc (const struct explicit_location *loc)
> + {
> + initialize_explicit_location (&explicit_loc);
> + if (loc != nullptr)
> + {
> + explicit_loc.func_name_match_type = loc->func_name_match_type;
> + if (loc->source_filename != nullptr)
> + explicit_loc.source_filename = xstrdup (loc->source_filename);
> + if (loc->function_name != nullptr)
> + explicit_loc.function_name = xstrdup (loc->function_name);
> + if (loc->label_name != nullptr)
> + explicit_loc.label_name = xstrdup (loc->label_name);
> + explicit_loc.line_offset = loc->line_offset;
> + }
> + }
> };
>
> /* See description in location.h. */
> @@ -82,23 +317,8 @@ event_location_up
> new_linespec_location (const char **linespec,
> symbol_name_match_type match_type)
> {
> - struct event_location *location;
> -
> - location = XCNEW (struct event_location);
> - location->type = LINESPEC_LOCATION;
> - location->u.linespec_location.match_type = match_type;
> - if (*linespec != NULL)
> - {
> - const char *p;
> - const char *orig = *linespec;
> -
> - linespec_lex_to_end (linespec);
> - p = remove_trailing_whitespace (orig, *linespec);
> - if ((p - orig) > 0)
> - location->u.linespec_location.spec_string
> - = savestring (orig, p - orig);
> - }
> - return event_location_up (location);
> + return event_location_up (new event_location_linespec (linespec,
> + match_type));
> }
>
> /* See description in location.h. */
> @@ -107,7 +327,7 @@ const linespec_location *
> get_linespec_location (const struct event_location *location)
> {
> gdb_assert (location->type == LINESPEC_LOCATION);
> - return &location->u.linespec_location;
> + return &((event_location_linespec *) location)->linespec_location;
> }
>
> /* See description in location.h. */
> @@ -116,14 +336,8 @@ event_location_up
> new_address_location (CORE_ADDR addr, const char *addr_string,
> int addr_string_len)
> {
> - struct event_location *location;
> -
> - location = XCNEW (struct event_location);
> - location->type = ADDRESS_LOCATION;
> - location->u.address = addr;
> - if (addr_string != NULL)
> - location->as_string = xstrndup (addr_string, addr_string_len);
> - return event_location_up (location);
> + return event_location_up (new event_location_address (addr, addr_string,
> + addr_string_len));
> }
>
> /* See description in location.h. */
> @@ -132,7 +346,7 @@ CORE_ADDR
> get_address_location (const struct event_location *location)
> {
> gdb_assert (location->type == ADDRESS_LOCATION);
> - return location->u.address;
> + return ((event_location_address *) location)->address;
> }
>
> /* See description in location.h. */
> @@ -149,13 +363,7 @@ get_address_string_location (const struct event_location *location)
> event_location_up
> new_probe_location (const char *probe)
> {
> - struct event_location *location;
> -
> - location = XCNEW (struct event_location);
> - location->type = PROBE_LOCATION;
> - if (probe != NULL)
> - location->u.addr_string = xstrdup (probe);
> - return event_location_up (location);
> + return event_location_up (new event_location_probe (probe));
> }
>
> /* See description in location.h. */
> @@ -164,7 +372,7 @@ const char *
> get_probe_location (const struct event_location *location)
> {
> gdb_assert (location->type == PROBE_LOCATION);
> - return location->u.addr_string;
> + return ((event_location_probe *) location)->addr_string;
> }
>
> /* See description in location.h. */
> @@ -172,34 +380,7 @@ get_probe_location (const struct event_location *location)
> event_location_up
> new_explicit_location (const struct explicit_location *explicit_loc)
> {
> - struct event_location tmp;
> -
> - memset (&tmp, 0, sizeof (struct event_location));
> - tmp.type = EXPLICIT_LOCATION;
> - initialize_explicit_location (&tmp.u.explicit_loc);
> - if (explicit_loc != NULL)
> - {
> - tmp.u.explicit_loc.func_name_match_type
> - = explicit_loc->func_name_match_type;
> -
> - if (explicit_loc->source_filename != NULL)
> - {
> - tmp.u.explicit_loc.source_filename
> - = explicit_loc->source_filename;
> - }
> -
> - if (explicit_loc->function_name != NULL)
> - tmp.u.explicit_loc.function_name
> - = explicit_loc->function_name;
> -
> - if (explicit_loc->label_name != NULL)
> - tmp.u.explicit_loc.label_name = explicit_loc->label_name;
> -
> - if (explicit_loc->line_offset.sign != LINE_OFFSET_UNKNOWN)
> - tmp.u.explicit_loc.line_offset = explicit_loc->line_offset;
> - }
> -
> - return copy_event_location (&tmp);
> + return event_location_up (new event_location_explicit (explicit_loc));
> }
>
> /* See description in location.h. */
> @@ -208,7 +389,7 @@ struct explicit_location *
> get_explicit_location (struct event_location *location)
> {
> gdb_assert (location->type == EXPLICIT_LOCATION);
> - return &location->u.explicit_loc;
> + return &((event_location_explicit *) location)->explicit_loc;
> }
>
> /* See description in location.h. */
> @@ -217,7 +398,7 @@ const struct explicit_location *
> get_explicit_location_const (const struct event_location *location)
> {
> gdb_assert (location->type == EXPLICIT_LOCATION);
> - return &location->u.explicit_loc;
> + return &((event_location_explicit *) location)->explicit_loc;
> }
>
> /* This convenience function returns a malloc'd string which
> @@ -301,91 +482,13 @@ explicit_location_to_linespec (const struct explicit_location *explicit_loc)
> event_location_up
> copy_event_location (const struct event_location *src)
> {
> - struct event_location *dst;
> -
> - dst = XCNEW (struct event_location);
> - dst->type = src->type;
> - if (src->as_string != NULL)
> - dst->as_string = xstrdup (src->as_string);
> -
> - switch (src->type)
> - {
> - case LINESPEC_LOCATION:
> - dst->u.linespec_location.match_type
> - = src->u.linespec_location.match_type;
> - if (src->u.linespec_location.spec_string != NULL)
> - dst->u.linespec_location.spec_string
> - = xstrdup (src->u.linespec_location.spec_string);
> - break;
> -
> - case ADDRESS_LOCATION:
> - dst->u.address = src->u.address;
> - break;
> -
> - case EXPLICIT_LOCATION:
> - dst->u.explicit_loc.func_name_match_type
> - = src->u.explicit_loc.func_name_match_type;
> - if (src->u.explicit_loc.source_filename != NULL)
> - dst->u.explicit_loc.source_filename
> - = xstrdup (src->u.explicit_loc.source_filename);
> -
> - if (src->u.explicit_loc.function_name != NULL)
> - dst->u.explicit_loc.function_name
> - = xstrdup (src->u.explicit_loc.function_name);
> -
> - if (src->u.explicit_loc.label_name != NULL)
> - dst->u.explicit_loc.label_name
> - = xstrdup (src->u.explicit_loc.label_name);
> -
> - dst->u.explicit_loc.line_offset = src->u.explicit_loc.line_offset;
> - break;
> -
> -
> - case PROBE_LOCATION:
> - if (src->u.addr_string != NULL)
> - dst->u.addr_string = xstrdup (src->u.addr_string);
> - break;
> -
> - default:
> - gdb_assert_not_reached ("unknown event location type");
> - }
> -
> - return event_location_up (dst);
> + return src->clone ();
> }
>
> void
> event_location_deleter::operator() (event_location *location) const
> {
> - if (location != NULL)
> - {
> - xfree (location->as_string);
> -
> - switch (location->type)
> - {
> - case LINESPEC_LOCATION:
> - xfree (location->u.linespec_location.spec_string);
> - break;
> -
> - case ADDRESS_LOCATION:
> - /* Nothing to do. */
> - break;
> -
> - case EXPLICIT_LOCATION:
> - xfree (location->u.explicit_loc.source_filename);
> - xfree (location->u.explicit_loc.function_name);
> - xfree (location->u.explicit_loc.label_name);
> - break;
> -
> - case PROBE_LOCATION:
> - xfree (location->u.addr_string);
> - break;
> -
> - default:
> - gdb_assert_not_reached ("unknown event location type");
> - }
> -
> - xfree (location);
> - }
> + delete location;
> }
>
> /* See description in location.h. */
> @@ -393,48 +496,7 @@ event_location_deleter::operator() (event_location *location) const
> const char *
> event_location_to_string (struct event_location *location)
> {
> - if (location->as_string == NULL)
> - {
> - switch (location->type)
> - {
> - case LINESPEC_LOCATION:
> - if (location->u.linespec_location.spec_string != NULL)
> - {
> - linespec_location *ls = &location->u.linespec_location;
> - if (ls->match_type == symbol_name_match_type::FULL)
> - {
> - location->as_string
> - = concat ("-qualified ", ls->spec_string, (char *) NULL);
> - }
> - else
> - location->as_string = xstrdup (ls->spec_string);
> - }
> - break;
> -
> - case ADDRESS_LOCATION:
> - {
> - const char *addr_string
> - = core_addr_to_string (location->u.address);
> - location->as_string
> - = xstrprintf ("*%s", addr_string).release ();
> - }
> - break;
> -
> - case EXPLICIT_LOCATION:
> - location->as_string
> - = explicit_location_to_string (&location->u.explicit_loc).release ();
> - break;
> -
> - case PROBE_LOCATION:
> - location->as_string = xstrdup (location->u.addr_string);
> - break;
> -
> - default:
> - gdb_assert_not_reached ("unknown event location type");
> - }
> - }
> -
> - return location->as_string;
> + return location->to_string ();
> }
>
> /* Find an instance of the quote character C in the string S that is
> @@ -720,8 +782,6 @@ string_to_explicit_location (const char **argp,
> const struct language_defn *language,
> explicit_completion_info *completion_info)
> {
> - event_location_up location;
> -
> /* It is assumed that input beginning with '-' and a non-digit
> character is an explicit location. "-p" is reserved, though,
> for probe locations. */
> @@ -732,7 +792,8 @@ string_to_explicit_location (const char **argp,
> || ((*argp)[0] == '-' && (*argp)[1] == 'p'))
> return NULL;
>
> - location = new_explicit_location (NULL);
> + std::unique_ptr<event_location_explicit> location
> + (new event_location_explicit ((const explicit_location *) nullptr));
>
> /* Process option/argument pairs. dprintf_command
> requires that processing stop on ','. */
> @@ -801,17 +862,17 @@ string_to_explicit_location (const char **argp,
> {
> set_oarg (explicit_location_lex_one (argp, language,
> completion_info));
> - location->u.explicit_loc.source_filename = oarg.release ();
> + location->explicit_loc.source_filename = oarg.release ();
> }
> else if (strncmp (opt.get (), "-function", len) == 0)
> {
> set_oarg (explicit_location_lex_one_function (argp, language,
> completion_info));
> - location->u.explicit_loc.function_name = oarg.release ();
> + location->explicit_loc.function_name = oarg.release ();
> }
> else if (strncmp (opt.get (), "-qualified", len) == 0)
> {
> - location->u.explicit_loc.func_name_match_type
> + location->explicit_loc.func_name_match_type
> = symbol_name_match_type::FULL;
> }
> else if (strncmp (opt.get (), "-line", len) == 0)
> @@ -820,7 +881,7 @@ string_to_explicit_location (const char **argp,
> *argp = skip_spaces (*argp);
> if (have_oarg)
> {
> - location->u.explicit_loc.line_offset
> + location->explicit_loc.line_offset
> = linespec_parse_line_offset (oarg.get ());
> continue;
> }
> @@ -828,7 +889,7 @@ string_to_explicit_location (const char **argp,
> else if (strncmp (opt.get (), "-label", len) == 0)
> {
> set_oarg (explicit_location_lex_one (argp, language, completion_info));
> - location->u.explicit_loc.label_name = oarg.release ();
> + location->explicit_loc.label_name = oarg.release ();
> }
> /* Only emit an "invalid argument" error for options
> that look like option strings. */
> @@ -858,17 +919,17 @@ string_to_explicit_location (const char **argp,
>
> /* One special error check: If a source filename was given
> without offset, function, or label, issue an error. */
> - if (location->u.explicit_loc.source_filename != NULL
> - && location->u.explicit_loc.function_name == NULL
> - && location->u.explicit_loc.label_name == NULL
> - && (location->u.explicit_loc.line_offset.sign == LINE_OFFSET_UNKNOWN)
> + if (location->explicit_loc.source_filename != NULL
> + && location->explicit_loc.function_name == NULL
> + && location->explicit_loc.label_name == NULL
> + && (location->explicit_loc.line_offset.sign == LINE_OFFSET_UNKNOWN)
> && completion_info == NULL)
> {
> error (_("Source filename requires function, label, or "
> "line offset."));
> }
>
> - return location;
> + return event_location_up (location.release ());
> }
>
> /* See description in location.h. */
> @@ -937,7 +998,10 @@ string_to_event_location (const char **stringp,
> "-qualified", otherwise string_to_explicit_location would
> have thrown an error. Save the flags for "basic" linespec
> parsing below and discard the explicit location. */
> - match_type = location->u.explicit_loc.func_name_match_type;
> + event_location_explicit *xloc
> + = dynamic_cast<event_location_explicit *> (location.get ());
> + gdb_assert (xloc != nullptr);
> + match_type = xloc->explicit_loc.func_name_match_type;
> }
>
> /* Everything else is a "basic" linespec, address, or probe
> @@ -950,28 +1014,7 @@ string_to_event_location (const char **stringp,
> int
> event_location_empty_p (const struct event_location *location)
> {
> - switch (location->type)
> - {
> - case LINESPEC_LOCATION:
> - /* Linespecs are never "empty." (NULL is a valid linespec) */
> - return 0;
> -
> - case ADDRESS_LOCATION:
> - return 0;
> -
> - case EXPLICIT_LOCATION:
> - return (location->u.explicit_loc.source_filename == NULL
> - && location->u.explicit_loc.function_name == NULL
> - && location->u.explicit_loc.label_name == NULL
> - && (location->u.explicit_loc.line_offset.sign
> - == LINE_OFFSET_UNKNOWN));
> -
> - case PROBE_LOCATION:
> - return location->u.addr_string == NULL;
> -
> - default:
> - gdb_assert_not_reached ("unknown event location type");
> - }
> + return location->empty_p ();
> }
>
> /* See description in location.h. */
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/6] C++-ify location.c
2022-01-14 16:50 [PATCH 0/6] C++-ify location.c Tom Tromey
` (5 preceding siblings ...)
2022-01-14 16:50 ` [PATCH 6/6] Simplify event_location_probe Tom Tromey
@ 2022-01-18 10:36 ` Andrew Burgess
6 siblings, 0 replies; 10+ messages in thread
From: Andrew Burgess @ 2022-01-18 10:36 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
* Tom Tromey <tom@tromey.com> [2022-01-14 09:50:02 -0700]:
> My foray into breakpoint.c led to me location.c. This patch cleans it
> up a bit, by C++-ifying the code. This removes some explicit memory
> management and removes some copies. It also, I think, reduces the
> chances for errors here by removing the use of a union in favor of
> subclassing.
>
> This code could be cleaned up a bit more. For example,
> explicit_location claims its fields are malloc'd, but this is not
> universally the case. I haven't attempted that.
>
> Regression tested on x86-64 Fedora 34.
I took a look through this series. I had one comment, but otherwise,
LGTM.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/6] Split event_location into subclasses
2022-01-18 10:33 ` Andrew Burgess
@ 2022-01-18 16:59 ` Tom Tromey
0 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2022-01-18 16:59 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches
>> /* An event location used to set a stop event in the inferior.
>> This structure is an amalgam of the various ways
>> to specify where a stop event should be set. */
Andrew> I think this comment is now out of date.
I'll update it.
>> - union
>> + /* Cached string representation of this location. This is used, e.g., to
>> + save stop event locations to file. Malloc'd. */
>> + char *as_string = nullptr;
Andrew> I wonder if you considered making these private, at least for type,
Andrew> and adding a read accessor? For as_string making it protected would
Andrew> seem like a better choice, we still need write access from
Andrew> set_event_location_string, but that itself could become a member
Andrew> function, and get_address_string_location would currently seem to need
Andrew> an accessor, though I notice in a later patch you change
Andrew> get_address_string_location to call to_string.
Andrew> If you think these changes would be better done later then this LGTM.
I didn't really consider it, but yeah, it makes sense.
I'd rather do this as a follow-up, though.
Tom
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-18 16:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-14 16:50 [PATCH 0/6] C++-ify location.c Tom Tromey
2022-01-14 16:50 ` [PATCH 1/6] Remove a use of xfree in location.c Tom Tromey
2022-01-14 16:50 ` [PATCH 2/6] Boolify explicit_to_string_internal Tom Tromey
2022-01-14 16:50 ` [PATCH 3/6] Remove EL_* macros from location.c Tom Tromey
2022-01-14 16:50 ` [PATCH 4/6] Split event_location into subclasses Tom Tromey
2022-01-18 10:33 ` Andrew Burgess
2022-01-18 16:59 ` Tom Tromey
2022-01-14 16:50 ` [PATCH 5/6] Use std::string in event_location Tom Tromey
2022-01-14 16:50 ` [PATCH 6/6] Simplify event_location_probe Tom Tromey
2022-01-18 10:36 ` [PATCH 0/6] C++-ify location.c Andrew Burgess
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).