* [PATCH] C++-ify path substitution code
@ 2021-12-10 19:27 Tom Tromey
2021-12-11 7:38 ` Joel Brobecker
0 siblings, 1 reply; 3+ messages in thread
From: Tom Tromey @ 2021-12-10 19:27 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
I found some uses of xfree in the path substitution code in source.c.
C++-ifying struct substitute_path_rule both simplifies the code and
removes manual memory management.
Regression tested on x86-64 Fedora 34.
---
gdb/source.c | 171 ++++++++++++++++-----------------------------------
gdb/source.h | 2 +-
2 files changed, 53 insertions(+), 120 deletions(-)
diff --git a/gdb/source.c b/gdb/source.c
index f28c30628e6..8a555dfc246 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -63,12 +63,17 @@ std::string source_path;
struct substitute_path_rule
{
- char *from;
- char *to;
- struct substitute_path_rule *next;
+ substitute_path_rule (const char *from_, const char *to_)
+ : from (from_),
+ to (to_)
+ {
+ }
+
+ std::string from;
+ std::string to;
};
-static struct substitute_path_rule *substitute_path_rules = NULL;
+static std::list<substitute_path_rule> substitute_path_rules;
/* An instance of this is attached to each program space. */
@@ -988,7 +993,7 @@ static int
substitute_path_rule_matches (const struct substitute_path_rule *rule,
const char *path)
{
- const int from_len = strlen (rule->from);
+ const int from_len = rule->from.length ();
const int path_len = strlen (path);
if (path_len < from_len)
@@ -997,7 +1002,7 @@ substitute_path_rule_matches (const struct substitute_path_rule *rule,
/* The substitution rules are anchored at the start of the path,
so the path should start with rule->from. */
- if (filename_ncmp (path, rule->from, from_len) != 0)
+ if (filename_ncmp (path, rule->from.c_str (), from_len) != 0)
return 0;
/* Make sure that the region in the path that matches the substitution
@@ -1016,12 +1021,11 @@ substitute_path_rule_matches (const struct substitute_path_rule *rule,
static struct substitute_path_rule *
get_substitute_path_rule (const char *path)
{
- struct substitute_path_rule *rule = substitute_path_rules;
+ for (substitute_path_rule &rule : substitute_path_rules)
+ if (substitute_path_rule_matches (&rule, path))
+ return &rule;
- while (rule != NULL && !substitute_path_rule_matches (rule, path))
- rule = rule->next;
-
- return rule;
+ return nullptr;
}
/* If the user specified a source path substitution rule that applies
@@ -1034,22 +1038,14 @@ gdb::unique_xmalloc_ptr<char>
rewrite_source_path (const char *path)
{
const struct substitute_path_rule *rule = get_substitute_path_rule (path);
- char *new_path;
- int from_len;
-
+
if (rule == NULL)
return NULL;
- from_len = strlen (rule->from);
-
/* Compute the rewritten path and return it. */
- new_path =
- (char *) xmalloc (strlen (path) + 1 + strlen (rule->to) - from_len);
- strcpy (new_path, rule->to);
- strcat (new_path, path + from_len);
-
- return gdb::unique_xmalloc_ptr<char> (new_path);
+ return (gdb::unique_xmalloc_ptr<char>
+ (concat (rule->to.c_str (), path + rule->from.length (), nullptr)));
}
/* See source.h. */
@@ -1730,79 +1726,13 @@ strip_trailing_directory_separator (char *path)
path[last] = '\0';
}
-/* Return the path substitution rule that matches FROM.
- Return NULL if no rule matches. */
-
-static struct substitute_path_rule *
-find_substitute_path_rule (const char *from)
-{
- struct substitute_path_rule *rule = substitute_path_rules;
-
- while (rule != NULL)
- {
- if (FILENAME_CMP (rule->from, from) == 0)
- return rule;
- rule = rule->next;
- }
-
- return NULL;
-}
-
/* Add a new substitute-path rule at the end of the current list of rules.
The new rule will replace FROM into TO. */
void
-add_substitute_path_rule (char *from, char *to)
+add_substitute_path_rule (const char *from, const char *to)
{
- struct substitute_path_rule *rule;
- struct substitute_path_rule *new_rule = XNEW (struct substitute_path_rule);
-
- new_rule->from = xstrdup (from);
- new_rule->to = xstrdup (to);
- new_rule->next = NULL;
-
- /* If the list of rules are empty, then insert the new rule
- at the head of the list. */
-
- if (substitute_path_rules == NULL)
- {
- substitute_path_rules = new_rule;
- return;
- }
-
- /* Otherwise, skip to the last rule in our list and then append
- the new rule. */
-
- rule = substitute_path_rules;
- while (rule->next != NULL)
- rule = rule->next;
-
- rule->next = new_rule;
-}
-
-/* Remove the given source path substitution rule from the current list
- of rules. The memory allocated for that rule is also deallocated. */
-
-static void
-delete_substitute_path_rule (struct substitute_path_rule *rule)
-{
- if (rule == substitute_path_rules)
- substitute_path_rules = rule->next;
- else
- {
- struct substitute_path_rule *prev = substitute_path_rules;
-
- while (prev != NULL && prev->next != rule)
- prev = prev->next;
-
- gdb_assert (prev != NULL);
-
- prev->next = rule->next;
- }
-
- xfree (rule->from);
- xfree (rule->to);
- xfree (rule);
+ substitute_path_rules.emplace_back (from, to);
}
/* Implement the "show substitute-path" command. */
@@ -1810,7 +1740,6 @@ delete_substitute_path_rule (struct substitute_path_rule *rule)
static void
show_substitute_path_command (const char *args, int from_tty)
{
- struct substitute_path_rule *rule = substitute_path_rules;
char *from = NULL;
gdb_argv argv (args);
@@ -1831,11 +1760,11 @@ show_substitute_path_command (const char *args, int from_tty)
else
printf_filtered (_("List of all source path substitution rules:\n"));
- while (rule != NULL)
+ for (substitute_path_rule &rule : substitute_path_rules)
{
- if (from == NULL || substitute_path_rule_matches (rule, from) != 0)
- printf_filtered (" `%s' -> `%s'.\n", rule->from, rule->to);
- rule = rule->next;
+ if (from == NULL || substitute_path_rule_matches (&rule, from) != 0)
+ printf_filtered (" `%s' -> `%s'.\n", rule.from.c_str (),
+ rule.to.c_str ());
}
}
@@ -1844,10 +1773,8 @@ show_substitute_path_command (const char *args, int from_tty)
static void
unset_substitute_path_command (const char *args, int from_tty)
{
- struct substitute_path_rule *rule = substitute_path_rules;
gdb_argv argv (args);
char *from = NULL;
- int rule_found = 0;
/* This function takes either 0 or 1 argument. */
@@ -1868,24 +1795,27 @@ unset_substitute_path_command (const char *args, int from_tty)
/* Delete the rule matching the argument. No argument means that
all rules should be deleted. */
- while (rule != NULL)
+ if (from == nullptr)
+ substitute_path_rules.clear ();
+ else
{
- struct substitute_path_rule *next = rule->next;
-
- if (from == NULL || FILENAME_CMP (from, rule->from) == 0)
- {
- delete_substitute_path_rule (rule);
- rule_found = 1;
- }
-
- rule = next;
+ auto iter
+ = std::remove_if (substitute_path_rules.begin (),
+ substitute_path_rules.end (),
+ [&] (const substitute_path_rule &rule)
+ {
+ return FILENAME_CMP (from,
+ rule.from.c_str ()) == 0;
+ });
+ bool rule_found = iter != substitute_path_rules.end ();
+ substitute_path_rules.erase (iter, substitute_path_rules.end ());
+
+ /* If the user asked for a specific rule to be deleted but
+ we could not find it, then report an error. */
+
+ if (!rule_found)
+ error (_("No substitution rule defined for `%s'"), from);
}
-
- /* If the user asked for a specific rule to be deleted but
- we could not find it, then report an error. */
-
- if (from != NULL && !rule_found)
- error (_("No substitution rule defined for `%s'"), from);
forget_cached_source_info ();
}
@@ -1895,8 +1825,6 @@ unset_substitute_path_command (const char *args, int from_tty)
static void
set_substitute_path_command (const char *args, int from_tty)
{
- struct substitute_path_rule *rule;
-
gdb_argv argv (args);
if (argv == NULL || argv[0] == NULL || argv [1] == NULL)
@@ -1916,10 +1844,15 @@ set_substitute_path_command (const char *args, int from_tty)
/* If a rule with the same "from" was previously defined, then
delete it. This new rule replaces it. */
- rule = find_substitute_path_rule (argv[0]);
- if (rule != NULL)
- delete_substitute_path_rule (rule);
-
+ auto iter
+ = std::remove_if (substitute_path_rules.begin (),
+ substitute_path_rules.end (),
+ [&] (const substitute_path_rule &rule)
+ {
+ return FILENAME_CMP (argv[0], rule.from.c_str ()) == 0;
+ });
+ substitute_path_rules.erase (iter, substitute_path_rules.end ());
+
/* Insert the new substitution rule. */
add_substitute_path_rule (argv[0], argv[1]);
diff --git a/gdb/source.h b/gdb/source.h
index 2b9e8f38c03..e146ce9d4a4 100644
--- a/gdb/source.h
+++ b/gdb/source.h
@@ -128,7 +128,7 @@ extern symtab_and_line set_current_source_symtab_and_line
extern void clear_current_source_symtab_and_line (void);
/* Add a source path substitution rule. */
-extern void add_substitute_path_rule (char *, char *);
+extern void add_substitute_path_rule (const char *, const char *);
/* Flags passed as 4th argument to print_source_lines. */
enum print_source_lines_flag
--
2.31.1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] C++-ify path substitution code
2021-12-10 19:27 [PATCH] C++-ify path substitution code Tom Tromey
@ 2021-12-11 7:38 ` Joel Brobecker
2021-12-12 17:56 ` Tom Tromey
0 siblings, 1 reply; 3+ messages in thread
From: Joel Brobecker @ 2021-12-11 7:38 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches, Joel Brobecker
Hi Tom,
On Fri, Dec 10, 2021 at 12:27:49PM -0700, Tom Tromey wrote:
> I found some uses of xfree in the path substitution code in source.c.
> C++-ifying struct substitute_path_rule both simplifies the code and
> removes manual memory management.
A nice simplification of the code thanks to the C++ std library!
Just one minor nit that I saw (NULL vs nullptr). Other than that,
thanks for the reminder-lesson about remove_if! ;-)
>
> Regression tested on x86-64 Fedora 34.
> ---
> gdb/source.c | 171 ++++++++++++++++-----------------------------------
> gdb/source.h | 2 +-
> 2 files changed, 53 insertions(+), 120 deletions(-)
>
> diff --git a/gdb/source.c b/gdb/source.c
> index f28c30628e6..8a555dfc246 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -63,12 +63,17 @@ std::string source_path;
>
> struct substitute_path_rule
> {
> - char *from;
> - char *to;
> - struct substitute_path_rule *next;
> + substitute_path_rule (const char *from_, const char *to_)
> + : from (from_),
> + to (to_)
> + {
> + }
> +
> + std::string from;
> + std::string to;
> };
>
> -static struct substitute_path_rule *substitute_path_rules = NULL;
> +static std::list<substitute_path_rule> substitute_path_rules;
>
> /* An instance of this is attached to each program space. */
>
> @@ -988,7 +993,7 @@ static int
> substitute_path_rule_matches (const struct substitute_path_rule *rule,
> const char *path)
> {
> - const int from_len = strlen (rule->from);
> + const int from_len = rule->from.length ();
> const int path_len = strlen (path);
>
> if (path_len < from_len)
> @@ -997,7 +1002,7 @@ substitute_path_rule_matches (const struct substitute_path_rule *rule,
> /* The substitution rules are anchored at the start of the path,
> so the path should start with rule->from. */
>
> - if (filename_ncmp (path, rule->from, from_len) != 0)
> + if (filename_ncmp (path, rule->from.c_str (), from_len) != 0)
> return 0;
>
> /* Make sure that the region in the path that matches the substitution
> @@ -1016,12 +1021,11 @@ substitute_path_rule_matches (const struct substitute_path_rule *rule,
> static struct substitute_path_rule *
> get_substitute_path_rule (const char *path)
> {
> - struct substitute_path_rule *rule = substitute_path_rules;
> + for (substitute_path_rule &rule : substitute_path_rules)
> + if (substitute_path_rule_matches (&rule, path))
> + return &rule;
>
> - while (rule != NULL && !substitute_path_rule_matches (rule, path))
> - rule = rule->next;
> -
> - return rule;
> + return nullptr;
> }
>
> /* If the user specified a source path substitution rule that applies
> @@ -1034,22 +1038,14 @@ gdb::unique_xmalloc_ptr<char>
> rewrite_source_path (const char *path)
> {
> const struct substitute_path_rule *rule = get_substitute_path_rule (path);
> - char *new_path;
> - int from_len;
> -
> +
> if (rule == NULL)
Change that to nullptr?
> return NULL;
>
> - from_len = strlen (rule->from);
> -
> /* Compute the rewritten path and return it. */
>
> - new_path =
> - (char *) xmalloc (strlen (path) + 1 + strlen (rule->to) - from_len);
> - strcpy (new_path, rule->to);
> - strcat (new_path, path + from_len);
> -
> - return gdb::unique_xmalloc_ptr<char> (new_path);
> + return (gdb::unique_xmalloc_ptr<char>
> + (concat (rule->to.c_str (), path + rule->from.length (), nullptr)));
> }
>
> /* See source.h. */
> @@ -1730,79 +1726,13 @@ strip_trailing_directory_separator (char *path)
> path[last] = '\0';
> }
>
> -/* Return the path substitution rule that matches FROM.
> - Return NULL if no rule matches. */
> -
> -static struct substitute_path_rule *
> -find_substitute_path_rule (const char *from)
> -{
> - struct substitute_path_rule *rule = substitute_path_rules;
> -
> - while (rule != NULL)
> - {
> - if (FILENAME_CMP (rule->from, from) == 0)
> - return rule;
> - rule = rule->next;
> - }
> -
> - return NULL;
> -}
> -
> /* Add a new substitute-path rule at the end of the current list of rules.
> The new rule will replace FROM into TO. */
>
> void
> -add_substitute_path_rule (char *from, char *to)
> +add_substitute_path_rule (const char *from, const char *to)
> {
> - struct substitute_path_rule *rule;
> - struct substitute_path_rule *new_rule = XNEW (struct substitute_path_rule);
> -
> - new_rule->from = xstrdup (from);
> - new_rule->to = xstrdup (to);
> - new_rule->next = NULL;
> -
> - /* If the list of rules are empty, then insert the new rule
> - at the head of the list. */
> -
> - if (substitute_path_rules == NULL)
> - {
> - substitute_path_rules = new_rule;
> - return;
> - }
> -
> - /* Otherwise, skip to the last rule in our list and then append
> - the new rule. */
> -
> - rule = substitute_path_rules;
> - while (rule->next != NULL)
> - rule = rule->next;
> -
> - rule->next = new_rule;
> -}
> -
> -/* Remove the given source path substitution rule from the current list
> - of rules. The memory allocated for that rule is also deallocated. */
> -
> -static void
> -delete_substitute_path_rule (struct substitute_path_rule *rule)
> -{
> - if (rule == substitute_path_rules)
> - substitute_path_rules = rule->next;
> - else
> - {
> - struct substitute_path_rule *prev = substitute_path_rules;
> -
> - while (prev != NULL && prev->next != rule)
> - prev = prev->next;
> -
> - gdb_assert (prev != NULL);
> -
> - prev->next = rule->next;
> - }
> -
> - xfree (rule->from);
> - xfree (rule->to);
> - xfree (rule);
> + substitute_path_rules.emplace_back (from, to);
> }
>
> /* Implement the "show substitute-path" command. */
> @@ -1810,7 +1740,6 @@ delete_substitute_path_rule (struct substitute_path_rule *rule)
> static void
> show_substitute_path_command (const char *args, int from_tty)
> {
> - struct substitute_path_rule *rule = substitute_path_rules;
> char *from = NULL;
>
> gdb_argv argv (args);
> @@ -1831,11 +1760,11 @@ show_substitute_path_command (const char *args, int from_tty)
> else
> printf_filtered (_("List of all source path substitution rules:\n"));
>
> - while (rule != NULL)
> + for (substitute_path_rule &rule : substitute_path_rules)
> {
> - if (from == NULL || substitute_path_rule_matches (rule, from) != 0)
> - printf_filtered (" `%s' -> `%s'.\n", rule->from, rule->to);
> - rule = rule->next;
> + if (from == NULL || substitute_path_rule_matches (&rule, from) != 0)
> + printf_filtered (" `%s' -> `%s'.\n", rule.from.c_str (),
> + rule.to.c_str ());
> }
> }
>
> @@ -1844,10 +1773,8 @@ show_substitute_path_command (const char *args, int from_tty)
> static void
> unset_substitute_path_command (const char *args, int from_tty)
> {
> - struct substitute_path_rule *rule = substitute_path_rules;
> gdb_argv argv (args);
> char *from = NULL;
> - int rule_found = 0;
>
> /* This function takes either 0 or 1 argument. */
>
> @@ -1868,24 +1795,27 @@ unset_substitute_path_command (const char *args, int from_tty)
> /* Delete the rule matching the argument. No argument means that
> all rules should be deleted. */
>
> - while (rule != NULL)
> + if (from == nullptr)
> + substitute_path_rules.clear ();
> + else
> {
> - struct substitute_path_rule *next = rule->next;
> -
> - if (from == NULL || FILENAME_CMP (from, rule->from) == 0)
> - {
> - delete_substitute_path_rule (rule);
> - rule_found = 1;
> - }
> -
> - rule = next;
> + auto iter
> + = std::remove_if (substitute_path_rules.begin (),
> + substitute_path_rules.end (),
> + [&] (const substitute_path_rule &rule)
> + {
> + return FILENAME_CMP (from,
> + rule.from.c_str ()) == 0;
> + });
> + bool rule_found = iter != substitute_path_rules.end ();
> + substitute_path_rules.erase (iter, substitute_path_rules.end ());
Thank the heavens for cppreference.com and their examples...
Clearly ultra-classic C++ and yet so obscure until you format
your brain to realize the remove_if is not removing anything!
> + /* If the user asked for a specific rule to be deleted but
> + we could not find it, then report an error. */
> +
> + if (!rule_found)
> + error (_("No substitution rule defined for `%s'"), from);
> }
> -
> - /* If the user asked for a specific rule to be deleted but
> - we could not find it, then report an error. */
> -
> - if (from != NULL && !rule_found)
> - error (_("No substitution rule defined for `%s'"), from);
>
> forget_cached_source_info ();
> }
> @@ -1895,8 +1825,6 @@ unset_substitute_path_command (const char *args, int from_tty)
> static void
> set_substitute_path_command (const char *args, int from_tty)
> {
> - struct substitute_path_rule *rule;
> -
> gdb_argv argv (args);
>
> if (argv == NULL || argv[0] == NULL || argv [1] == NULL)
> @@ -1916,10 +1844,15 @@ set_substitute_path_command (const char *args, int from_tty)
> /* If a rule with the same "from" was previously defined, then
> delete it. This new rule replaces it. */
>
> - rule = find_substitute_path_rule (argv[0]);
> - if (rule != NULL)
> - delete_substitute_path_rule (rule);
> -
> + auto iter
> + = std::remove_if (substitute_path_rules.begin (),
> + substitute_path_rules.end (),
> + [&] (const substitute_path_rule &rule)
> + {
> + return FILENAME_CMP (argv[0], rule.from.c_str ()) == 0;
> + });
> + substitute_path_rules.erase (iter, substitute_path_rules.end ());
> +
> /* Insert the new substitution rule. */
>
> add_substitute_path_rule (argv[0], argv[1]);
> diff --git a/gdb/source.h b/gdb/source.h
> index 2b9e8f38c03..e146ce9d4a4 100644
> --- a/gdb/source.h
> +++ b/gdb/source.h
> @@ -128,7 +128,7 @@ extern symtab_and_line set_current_source_symtab_and_line
> extern void clear_current_source_symtab_and_line (void);
>
> /* Add a source path substitution rule. */
> -extern void add_substitute_path_rule (char *, char *);
> +extern void add_substitute_path_rule (const char *, const char *);
>
> /* Flags passed as 4th argument to print_source_lines. */
> enum print_source_lines_flag
> --
> 2.31.1
>
--
Joel
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] C++-ify path substitution code
2021-12-11 7:38 ` Joel Brobecker
@ 2021-12-12 17:56 ` Tom Tromey
0 siblings, 0 replies; 3+ messages in thread
From: Tom Tromey @ 2021-12-12 17:56 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Tom Tromey, gdb-patches
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
>> I found some uses of xfree in the path substitution code in source.c.
>> C++-ifying struct substitute_path_rule both simplifies the code and
>> removes manual memory management.
Joel> A nice simplification of the code thanks to the C++ std library!
Joel> Just one minor nit that I saw (NULL vs nullptr). Other than that,
Joel> thanks for the reminder-lesson about remove_if! ;-)
>> if (rule == NULL)
Joel> Change that to nullptr?
I am checking it in with this change.
Tom
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-12 17:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-10 19:27 [PATCH] C++-ify path substitution code Tom Tromey
2021-12-11 7:38 ` Joel Brobecker
2021-12-12 17:56 ` Tom Tromey
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).