This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 1/3] Convert generic probe interface to C++ (and perform some cleanups)
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>, GDB Patches <gdb-patches at sourceware dot org>
- Cc: Simon Marchi <simark at simark dot ca>
- Date: Wed, 22 Nov 2017 15:39:15 -0500
- Subject: Re: [PATCH v2 1/3] Convert generic probe interface to C++ (and perform some cleanups)
- Authentication-results: sourceware.org; auth=none
- Authentication-results: spf=none (sender IP is ) smtp.mailfrom=simon dot marchi at ericsson dot com;
- References: <20171113175901.25367-1-sergiodj@redhat.com> <20171116043734.10137-1-sergiodj@redhat.com> <20171116043734.10137-2-sergiodj@redhat.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
On 2017-11-15 11:37 PM, Sergio Durigan Junior wrote:
> Changes from v1:
>
> - Remove emit_info_probes_extra_fields method.
>
> - Make gen_info_* methods return the vector directly. Modify code
> accordingly.
>
> - Cleanup unused variables.
>
> - Make can_evaluate_arguments return bool.
>
> - Make get_name and get_provider return a const std::string &. Modify
> code accordingly.
>
> - Move can_enable method from 'class probe' to 'class
> static_probe_ops'.
>
> - Use const references when iterating on vector of objects.
>
> This patch converts the generic probe interface (gdb/probe.[ch]) to
> C++, and also performs some cleanups that were on my TODO list for a
> while.
>
> The main changes were the conversion of 'struct probe' to 'class
> probe', and 'struct probe_ops' to 'class static_probe_ops'. The
> former now contains all the "dynamic", generic methods that act on a
> probe + the generic data related to it; the latter encapsulates a
> bunch of "static" methods that relate to the probe type, but not to a
> specific probe itself.
>
> I've had to do a few renamings (e.g., on 'struct bound_probe' the
> field is called 'probe *prob' now, instead of 'struct probe *probe')
> because GCC was complaining about naming the field using the same name
> as the class. Nothing major, though. Generally speaking, the logic
> behind and the design behind the code are the same.
>
> Even though I'm sending a series of patches, they need to be tested
> and committed as a single unit, because of inter-dependencies. But it
> should be easier to review in separate logical units.
>
> I've regtested this patch on BuildBot, no regressions found.
Hi Sergio,
I've looked at it (the series) quickly, and it looks good to me. Personally,
for comparing std::string with char*, I would use the form
str != char_ptr
rather than
str.compare (char_ptr) != 0
since I find it more readable, but I don't really mind.
One whitespace comment below.
> @@ -334,56 +358,33 @@ compare_probes (const bound_probe &a, const bound_probe &b)
>
> static void
> gen_ui_out_table_header_info (const std::vector<bound_probe> &probes,
> - const struct probe_ops *p)
> + const static_probe_ops *spops)
> {
> /* `headings' refers to the names of the columns when printing `info
> probes'. */
> - VEC (info_probe_column_s) *headings = NULL;
> - struct cleanup *c;
> - info_probe_column_s *column;
> - size_t headings_size;
> - int ix;
> + gdb_assert (spops != NULL);
>
> - gdb_assert (p != NULL);
> + std::vector<struct info_probe_column> headings
> + = spops->gen_info_probes_table_header ();
>
> - if (p->gen_info_probes_table_header == NULL
> - && p->gen_info_probes_table_values == NULL)
> - return;
> -
> - gdb_assert (p->gen_info_probes_table_header != NULL
> - && p->gen_info_probes_table_values != NULL);
> -
> - c = make_cleanup (VEC_cleanup (info_probe_column_s), &headings);
> - p->gen_info_probes_table_header (&headings);
> -
> - headings_size = VEC_length (info_probe_column_s, headings);
> -
> - for (ix = 0;
> - VEC_iterate (info_probe_column_s, headings, ix, column);
> - ++ix)
> + for (const struct info_probe_column &column : headings)
> {
> - size_t size_max = strlen (column->print_name);
> + size_t size_max = strlen (column.print_name);
>
> for (const bound_probe &probe : probes)
> {
> /* `probe_fields' refers to the values of each new field that this
> probe will display. */
> - VEC (const_char_ptr) *probe_fields = NULL;
> - struct cleanup *c2;
> - const char *val;
> - int kx;
>
> - if (probe.probe->pops != p)
> + if (probe.prob->get_static_ops () != spops)
> continue;
>
> - c2 = make_cleanup (VEC_cleanup (const_char_ptr), &probe_fields);
> - p->gen_info_probes_table_values (probe.probe, &probe_fields);
> + std::vector<const char *> probe_fields
There are spaces before the tab in this line.
Thanks,
Simon