[RFA,09/11] Use std::set in mi-main.c
Commit Message
Change a couple of spots in mi-main.c to use std::set. This
simplifies the code and removes some cleanups.
ChangeLog
2017-09-12 Tom Tromey <tom@tromey.com>
* mi/mi-main.c (struct print_one_inferior_data) <inferiors>: Now a
'std::set *'.
(print_one_inferior): Update.
(free_vector_of_ints): Remove.
(list_available_thread_groups): Change "ids" to std::set.
(mi_cmd_list_thread_groups): Update.
(struct collect_cores_data) <core>: Now a std::set.
(collect_cores): Update.
(unique): Remove.
(print_one_inferior): Update.
---
gdb/ChangeLog | 13 ++++++++++
gdb/mi/mi-main.c | 77 +++++++++++++-------------------------------------------
2 files changed, 31 insertions(+), 59 deletions(-)
Comments
On 09/12/2017 07:57 PM, Tom Tromey wrote:
> Change a couple of spots in mi-main.c to use std::set. This
> simplifies the code and removes some cleanups.
std::set always gives me pause. For small objects like int,
and when the use case is insertion phase + lookup phase + discard set,
unsorted inserting into a vector, sorting, and then binary searching
the vector for lookuups is very likely to have better performance, for
cache locality reasons, and also because fewer allocations (with
std::set being a node-based container...)
But it's likely that in this case it doesn't really matter, so let's
go with the simplicity argument.
(At some point I may propose some data structure on top of
std::vector for use cases like this.)
Patch is OK as is.
Thanks,
Pedro Alves
On 2017-09-28 12:10, Pedro Alves wrote:
> On 09/12/2017 07:57 PM, Tom Tromey wrote:
>> Change a couple of spots in mi-main.c to use std::set. This
>> simplifies the code and removes some cleanups.
>
> std::set always gives me pause. For small objects like int,
> and when the use case is insertion phase + lookup phase + discard set,
> unsorted inserting into a vector, sorting, and then binary searching
> the vector for lookuups is very likely to have better performance, for
> cache locality reasons, and also because fewer allocations (with
> std::set being a node-based container...)
>
> But it's likely that in this case it doesn't really matter, so let's
> go with the simplicity argument.
>
> (At some point I may propose some data structure on top of
> std::vector for use cases like this.)
We have discussed something like this with Sergio in this thread:
https://sourceware.org/ml/gdb-patches/2017-07/msg00434.html
(you can ctrl-f "boilerplate")
We managed to sneak in an std::set while you were not looking/on
vacation :). The idea was that the std::set interface really helped to
keep the code clean and short, and that we could later implement
something with the same behavior and interface as std::set, but based on
a vector. It's a bit different than what you propose, because to be a
drop-in replacement for a set, we would always keep the vector sorted
(insert the new elements where they belong). IIUC, what you propose is
to insert everything and then sort it. I think the latter has a better
worst-case performance (sorting at the end is n*log(n)) compared to the
former (insertion in reverse order will become n^2). I don't think it
matters much though, since this data structure would be explicitly for
relatively small amount of items.
I am not sure if the reasoning is different for a set of string (as in
common/environ.{h,c}). I assume it would be similar, since the actual
string object is rather small, and when inserting a string in the
middle, the following strings will be moved one position and not copied
(I haven't verified though).
Simon
On 2017-09-12 20:57, Tom Tromey wrote:
> static void
> -list_available_thread_groups (VEC (int) *ids, int recurse)
> +list_available_thread_groups (const std::set<int> &ids, int recurse)
> {
> struct osdata *data;
> struct osdata_item *item;
> @@ -824,12 +795,9 @@ list_available_thread_groups (VEC (int) *ids, int
> recurse)
> /* At present, the target will return all available processes
> and if information about specific ones was required, we filter
> undesired processes here. */
> - if (ids && bsearch (&pid_i, VEC_address (int, ids),
> - VEC_length (int, ids),
> - sizeof (int), compare_positive_ints) == NULL)
> + if (!ids.empty () && ids.find (pid_i) != ids.end ())
I think the condition is the wrong way, it should be == and not !=. It
probably means we don't have a test for this feature.
Simon
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
Simon> On 2017-09-12 20:57, Tom Tromey wrote:
>> static void
>> -list_available_thread_groups (VEC (int) *ids, int recurse)
>> +list_available_thread_groups (const std::set<int> &ids, int recurse)
>> {
>> struct osdata *data;
>> struct osdata_item *item;
>> @@ -824,12 +795,9 @@ list_available_thread_groups (VEC (int) *ids,
>> int recurse)
>> /* At present, the target will return all available processes
>> and if information about specific ones was required, we filter
>> undesired processes here. */
>> - if (ids && bsearch (&pid_i, VEC_address (int, ids),
>> - VEC_length (int, ids),
>> - sizeof (int), compare_positive_ints) == NULL)
>> + if (!ids.empty () && ids.find (pid_i) != ids.end ())
Simon> I think the condition is the wrong way, it should be == and not !=.
Simon> It probably means we don't have a test for this feature.
Yes, I think you're right. Changing it to == doesn't affect the gdb.mi
test results for me.
Tom
@@ -1,5 +1,18 @@
2017-09-12 Tom Tromey <tom@tromey.com>
+ * mi/mi-main.c (struct print_one_inferior_data) <inferiors>: Now a
+ 'std::set *'.
+ (print_one_inferior): Update.
+ (free_vector_of_ints): Remove.
+ (list_available_thread_groups): Change "ids" to std::set.
+ (mi_cmd_list_thread_groups): Update.
+ (struct collect_cores_data) <core>: Now a std::set.
+ (collect_cores): Update.
+ (unique): Remove.
+ (print_one_inferior): Update.
+
+2017-09-12 Tom Tromey <tom@tromey.com>
+
* mi/mi-main.c (mi_execute_cli_command): Use unique_xmalloc_ptr.
(mi_execute_async_cli_command): Likewise.
(mi_cmd_trace_frame_collected): Use std::string.
@@ -62,6 +62,8 @@
#include <chrono>
#include "progspace-and-thread.h"
#include "common/rsp-low.h"
+#include <algorithm>
+#include <set>
enum
{
@@ -604,8 +606,7 @@ mi_cmd_thread_info (const char *command, char **argv, int argc)
struct collect_cores_data
{
int pid;
-
- VEC (int) *cores;
+ std::set<int> cores;
};
static int
@@ -618,27 +619,16 @@ collect_cores (struct thread_info *ti, void *xdata)
int core = target_core_of_thread (ti->ptid);
if (core != -1)
- VEC_safe_push (int, data->cores, core);
+ data->cores.insert (core);
}
return 0;
}
-static int *
-unique (int *b, int *e)
-{
- int *d = b;
-
- while (++b != e)
- if (*d != *b)
- *++d = *b;
- return ++d;
-}
-
struct print_one_inferior_data
{
int recurse;
- VEC (int) *inferiors;
+ const std::set<int> *inferiors;
};
static int
@@ -648,10 +638,9 @@ print_one_inferior (struct inferior *inferior, void *xdata)
= (struct print_one_inferior_data *) xdata;
struct ui_out *uiout = current_uiout;
- if (VEC_empty (int, top_data->inferiors)
- || bsearch (&(inferior->pid), VEC_address (int, top_data->inferiors),
- VEC_length (int, top_data->inferiors), sizeof (int),
- compare_positive_ints))
+ if (top_data->inferiors->empty ()
+ || (top_data->inferiors->find (inferior->pid)
+ != top_data->inferiors->end ()))
{
struct collect_cores_data data;
ui_out_emit_tuple tuple_emitter (uiout, NULL);
@@ -670,28 +659,18 @@ print_one_inferior (struct inferior *inferior, void *xdata)
inferior->pspace->pspace_exec_filename);
}
- data.cores = 0;
if (inferior->pid != 0)
{
data.pid = inferior->pid;
iterate_over_threads (collect_cores, &data);
}
- if (!VEC_empty (int, data.cores))
+ if (!data.cores.empty ())
{
- int *b, *e;
ui_out_emit_list list_emitter (uiout, "cores");
- qsort (VEC_address (int, data.cores),
- VEC_length (int, data.cores), sizeof (int),
- compare_positive_ints);
-
- b = VEC_address (int, data.cores);
- e = b + VEC_length (int, data.cores);
- e = unique (b, e);
-
- for (; b != e; ++b)
- uiout->field_int (NULL, *b);
+ for (int b : data.cores)
+ uiout->field_int (NULL, b);
}
if (top_data->recurse)
@@ -717,14 +696,6 @@ output_cores (struct ui_out *uiout, const char *field_name, const char *xcores)
}
static void
-free_vector_of_ints (void *xvector)
-{
- VEC (int) **vector = (VEC (int) **) xvector;
-
- VEC_free (int, *vector);
-}
-
-static void
do_nothing (splay_tree_key k)
{
}
@@ -755,7 +726,7 @@ free_splay_tree (void *xt)
}
static void
-list_available_thread_groups (VEC (int) *ids, int recurse)
+list_available_thread_groups (const std::set<int> &ids, int recurse)
{
struct osdata *data;
struct osdata_item *item;
@@ -824,12 +795,9 @@ list_available_thread_groups (VEC (int) *ids, int recurse)
/* At present, the target will return all available processes
and if information about specific ones was required, we filter
undesired processes here. */
- if (ids && bsearch (&pid_i, VEC_address (int, ids),
- VEC_length (int, ids),
- sizeof (int), compare_positive_ints) == NULL)
+ if (!ids.empty () && ids.find (pid_i) != ids.end ())
continue;
-
ui_out_emit_tuple tuple_emitter (uiout, NULL);
uiout->field_fmt ("id", "%s", pid);
@@ -875,10 +843,9 @@ void
mi_cmd_list_thread_groups (const char *command, char **argv, int argc)
{
struct ui_out *uiout = current_uiout;
- struct cleanup *back_to;
int available = 0;
int recurse = 0;
- VEC (int) *ids = 0;
+ std::set<int> ids;
enum opt
{
@@ -930,23 +897,17 @@ mi_cmd_list_thread_groups (const char *command, char **argv, int argc)
if (*end != '\0')
error (_("invalid syntax of group id '%s'"), argv[oind]);
- VEC_safe_push (int, ids, inf);
+ ids.insert (inf);
}
- if (VEC_length (int, ids) > 1)
- qsort (VEC_address (int, ids),
- VEC_length (int, ids),
- sizeof (int), compare_positive_ints);
-
- back_to = make_cleanup (free_vector_of_ints, &ids);
if (available)
{
list_available_thread_groups (ids, recurse);
}
- else if (VEC_length (int, ids) == 1)
+ else if (ids.size () == 1)
{
/* Local thread groups, single id. */
- int id = *VEC_address (int, ids);
+ int id = *(ids.begin ());
struct inferior *inf = find_inferior_id (id);
if (!inf)
@@ -959,7 +920,7 @@ mi_cmd_list_thread_groups (const char *command, char **argv, int argc)
struct print_one_inferior_data data;
data.recurse = recurse;
- data.inferiors = ids;
+ data.inferiors = &ids;
/* Local thread groups. Either no explicit ids -- and we
print everything, or several explicit ids. In both cases,
@@ -969,8 +930,6 @@ mi_cmd_list_thread_groups (const char *command, char **argv, int argc)
update_thread_list ();
iterate_over_inferiors (print_one_inferior, &data);
}
-
- do_cleanups (back_to);
}
void