From: Sergio Durigan Junior <sergiodj@redhat.com>
To: Simon Marchi <simon.marchi@ericsson.com>
Cc: GDB Patches <gdb-patches@sourceware.org>,
Simon Marchi <simark@simark.ca>
Subject: Re: [PATCH v2 1/3] Convert generic probe interface to C++ (and perform some cleanups)
Date: Wed, 22 Nov 2017 22:36:00 -0000 [thread overview]
Message-ID: <87d14adjqg.fsf@redhat.com> (raw)
In-Reply-To: <d0f9b0fe-f3d7-97ed-4a51-b132105d1199@ericsson.com> (Simon Marchi's message of "Wed, 22 Nov 2017 15:39:15 -0500")
On Wednesday, November 22 2017, Simon Marchi wrote:
> 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,
Hey Simon,
Thanks for the review.
> 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.
You're right, it's better. I'll replace that in my code.
> 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 for catching that. Fixed.
Is it OK to push after these changes, or do you want me to submit a v2?
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
next prev parent reply other threads:[~2017-11-22 22:36 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-13 17:59 [PATCH 0/3] Convert probe interfaces " Sergio Durigan Junior
2017-11-13 17:59 ` [PATCH 1/3] Convert generic probe interface " Sergio Durigan Junior
2017-11-15 2:52 ` Simon Marchi
2017-11-15 3:25 ` Simon Marchi
2017-11-15 6:15 ` Sergio Durigan Junior
2017-11-15 6:12 ` Sergio Durigan Junior
2017-11-13 17:59 ` [PATCH 2/3] Convert SystemTap " Sergio Durigan Junior
2017-11-15 3:58 ` Simon Marchi
2017-11-15 22:49 ` Sergio Durigan Junior
2017-11-13 17:59 ` [PATCH 3/3] Convert DTrace " Sergio Durigan Junior
2017-11-15 4:40 ` Simon Marchi
2017-11-16 4:11 ` Sergio Durigan Junior
2017-11-16 4:37 ` [PATCH v2 0/3] Convert probe interfaces " Sergio Durigan Junior
2017-11-16 4:38 ` [PATCH v2 3/3] Convert DTrace probe interface " Sergio Durigan Junior
2017-11-16 4:38 ` [PATCH v2 2/3] Convert SystemTap " Sergio Durigan Junior
2017-11-16 4:38 ` [PATCH v2 1/3] Convert generic " Sergio Durigan Junior
2017-11-22 20:39 ` Simon Marchi
2017-11-22 22:36 ` Sergio Durigan Junior [this message]
2017-11-23 0:01 ` Simon Marchi
2017-11-23 0:15 ` Sergio Durigan Junior
2017-11-23 0:33 ` Sergio Durigan Junior
2017-11-21 16:25 ` [PATCH v2 0/3] Convert probe interfaces " Sergio Durigan Junior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87d14adjqg.fsf@redhat.com \
--to=sergiodj@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=simark@simark.ca \
--cc=simon.marchi@ericsson.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).