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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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/