Re: [PATCH v3 1/2] Emit inferior, thread and frame selection events to all UIs

[Simon Marchi <simon.marchi@ericsson.com>]

On 16-10-03 12:47 PM, Pedro Alves wrote:
> Hi Simon,
> 
> This looks good to me now, module a couple minor issues pointed out
> below.  Fix these and you're good to go.
> 
> On 09/24/2016 09:13 PM, Simon Marchi wrote:
>> @@ -1885,7 +1901,19 @@ void
>>  cmd_func (struct cmd_list_element *cmd, char *args, int from_tty)
>>  {
>>    if (cmd_func_p (cmd))
>> -    (*cmd->func) (cmd, args, from_tty);
>> +    {
>> +      struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
>> +
>> +      if (cmd->suppress_notification != NULL)
>> +	{
>> +	  cleanups = make_cleanup_restore_integer (cmd->suppress_notification);
> 
> This will incorrectly leave the null_cleanup not run.
> You should not overwrite "cleanups".  Should be just:
> 
>   +	  make_cleanup_restore_integer (cmd->suppress_notification);

Done, thanks.

>> +	  *cmd->suppress_notification = 1;
>> +	}
>> +
>> +      (*cmd->func) (cmd, args, from_tty);
>> +
>> +      do_cleanups (cleanups);
> 
> 
> 
>> +  else /* MI_COMMAND */
>> +    {
>> +      if (strcmp (command->command, "interpreter-exec") == 0
>> +	  && command->argc > 1)
>> +
> 
> This empty line here made me pause and think that the code
> looks suspicious.  Better would be to wrap the then/else blocks
> in {}s, since they're multi-line.

Right, it's clearer.

>> +@item =thread-selected,id="@var{id}"[,frame="@var{frame}"]
>> +Informs that the selected thread or frame were changed.  This notification
>> +is not emitted as result of the @code{-thread-select} or
>> +@code{-stack-select-frame} commands, but is emitted whenever an MI command
>> +that is not documented to change the selected thread and frame actually
>> +changes them.  In particular, invoking, directly or indirectly
>> +(via user-defined command), the CLI @code{thread} or @code{frame} commands,
>> +will generate this notification.  Changing the thread of frame from another
>> +user interface (see @ref{Interpreters}) will also generate this notification.
>> +
> 
> Typo: s/thread of frame/thread or frame/

Woops, thanks.

I'll give it a last complete test run before pushing.

Simon