From: Sergio Durigan Junior <sergiodj@redhat.com>
To: "Jose E. Marchesi" <jose.marchesi@oracle.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH 3/9] New commands `enable probe' and `disable probe'.
Date: Tue, 30 Sep 2014 23:20:00 -0000 [thread overview]
Message-ID: <87iok44rwb.fsf@redhat.com> (raw)
In-Reply-To: <87mw9g4s8q.fsf@redhat.com> (Sergio Durigan Junior's message of "Tue, 30 Sep 2014 19:13:09 -0400")
On Tuesday, September 30 2014, I wrote:
>> +/* Implementation of the `enable probes' command. */
>> +
>> +static void
>> +enable_probes_command (char *arg, int from_tty)
>> +{
>> + char *provider, *probe_name = NULL, *objname = NULL;
>> + struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
>> + VEC (bound_probe_s) *probes;
>> + struct bound_probe *probe;
>> + int i;
>> +
>> + /* Do we have a `provider:probe:objfile' style of linespec? */
>> + provider = extract_arg (&arg);
>> + if (provider)
>> + {
>> + make_cleanup (xfree, provider);
>> +
>> + probe_name = extract_arg (&arg);
>> + if (probe_name)
>> + {
>> + make_cleanup (xfree, probe_name);
>> +
>> + objname = extract_arg (&arg);
>> + if (objname)
>> + make_cleanup (xfree, objname);
>> + }
>> + }
>
> This construct has become useful enough that it can be put in a
> function, probably. Would you mind doing that?
>
>> + probes = collect_probes (objname, provider, probe_name, NULL /* pops */);
>
> When I code, I try not to put comments inside the parentheses. You can
> put a comment above the function, or just remove it at all (there is
> another call to collect_probes that doesn't have a comment).
>
>> + if (VEC_empty (bound_probe_s, probes))
>> + {
>> + ui_out_message (current_uiout, 0, _("No probes matched.\n"));
>> + return;
>> + }
>
> You should call do_cleanups before returning here (and in other places).
>
>> + /* Enable the selected probes, provided their backends support the
>> + notion of enabling a probe. */
>> + for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i)
>> + {
>> + const struct probe_ops *pops = probe->probe->pops;
>> + if (pops->enable_probe)
>
> Newline between variable declaration and code. You are also comparing
> things implicitly, and we do it explicitly:
>
> if (pops->enable_probe != NULL)
>
>> + {
>> + pops->enable_probe (probe->probe);
>> + ui_out_message (current_uiout, 0,
>> + _("Probe %s:%s enabled.\n"),
>> + probe->probe->provider, probe->probe->name);
>> + }
>> + }
>
> I would appreciate a warning here if pops->enable_probe == NULL, saying
> that the probe cannot be enabled/disabled.
BTW, I forgot to say, but you should call do_cleanups before the
function ends as well (the same applies for disable_probes_command).
--
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
next prev parent reply other threads:[~2014-09-30 23:20 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <no>
2012-04-18 9:27 ` [RFA] Add proper handling for internal functions and STT_GNU_IFUNC symbols in Ada mode Paul Hilfinger
2012-04-18 14:45 ` Joel Brobecker
2012-04-22 15:33 ` [committed] " Paul Hilfinger
2014-09-26 9:43 ` [PATCH 0/9] Add support for DTrace USDT probes to gdb Jose E. Marchesi
2014-09-26 9:43 ` [PATCH 4/9] New gdbarch functions: dtrace_probe_argument, dtrace_probe_is_enabled, dtrace_enable_probe, dtrace_disable_probe Jose E. Marchesi
2014-10-02 21:34 ` Sergio Durigan Junior
2014-10-10 16:38 ` Jose E. Marchesi
2014-09-26 9:43 ` [PATCH 2/9] Move `compute_probe_arg' and `compile_probe_arg' to probe.c Jose E. Marchesi
2014-09-30 0:02 ` Sergio Durigan Junior
2014-10-10 16:38 ` Jose E. Marchesi
2014-09-26 9:43 ` [PATCH 1/9] Adapt `info probes' to support printing probes of different types Jose E. Marchesi
2014-09-29 21:15 ` Sergio Durigan Junior
2014-10-10 16:38 ` Jose E. Marchesi
2014-09-26 9:43 ` [PATCH 7/9] Simple testsuite for DTrace USDT probes Jose E. Marchesi
2014-10-08 19:30 ` Sergio Durigan Junior
2014-10-10 16:38 ` Jose E. Marchesi
2014-09-26 9:43 ` [PATCH 9/9] Announce the DTrace USDT probes support in NEWS Jose E. Marchesi
2014-09-26 13:12 ` Eli Zaretskii
2014-09-29 10:29 ` Jose E. Marchesi
2014-09-26 9:43 ` [PATCH 6/9] Support for DTrace USDT probes in x86_64 targets Jose E. Marchesi
2014-10-08 19:32 ` Sergio Durigan Junior
2014-10-10 16:38 ` Jose E. Marchesi
2014-09-26 9:43 ` [PATCH 3/9] New commands `enable probe' and `disable probe' Jose E. Marchesi
2014-09-26 13:11 ` Eli Zaretskii
2014-09-29 10:26 ` Jose E. Marchesi
2014-09-30 23:13 ` Sergio Durigan Junior
2014-09-30 23:20 ` Sergio Durigan Junior [this message]
2014-10-10 16:38 ` Jose E. Marchesi
2014-09-26 9:43 ` [PATCH 5/9] New probe type: DTrace USDT probes Jose E. Marchesi
2014-09-26 13:19 ` Eli Zaretskii
2014-10-02 23:19 ` Sergio Durigan Junior
2014-10-10 16:38 ` Jose E. Marchesi
2014-10-10 18:13 ` Sergio Durigan Junior
2014-10-10 18:32 ` Jose E. Marchesi
2014-10-10 18:44 ` Sergio Durigan Junior
2014-09-26 9:43 ` [PATCH 8/9] Documentation for " Jose E. Marchesi
2014-09-26 13:18 ` Eli Zaretskii
2014-09-29 10:26 ` Jose E. Marchesi
2014-09-29 13:35 ` Eli Zaretskii
2014-09-29 13:53 ` Jose E. Marchesi
2014-10-08 19:40 ` [PATCH 0/9] Add support for DTrace USDT probes to gdb Sergio Durigan Junior
2014-10-09 8:05 ` Jose E. Marchesi
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=87iok44rwb.fsf@redhat.com \
--to=sergiodj@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=jose.marchesi@oracle.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).