Re: [PATCH 3/9] New commands `enable probe' and `disable probe'.

Sergio Durigan Junior
2014-09-30T23:20:42Z

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