Re: [PATCH 3/3] suppress notification

[Pedro Alves <palves at redhat dot com>]

On 08/28/2012 02:09 PM, Yao Qi wrote:
> On 08/28/2012 07:56 PM, Vladimir Prus wrote:
>> Well, the problem is that this is not a generic mechanism to everybody
>> to know whether command X is presently running -- because
>> this mechanism can set only one variable, and for some commands that
>> variable is already notification flag.
>>
> 
> If we want to know whether command X is running, we can add more fields in 'struct mi_suppress_notification', and each field is associated with one command in this way (set 'called' point to the address of field in 'struct mi_suppress_notificatin').  It is unnecessary according to current requirement, and it can evolve easily once we have such requirement in the future.
> 
>>> If you still think it is misleading, I'd like to rename variable
>>> 'mi_suppress_notification' to 'mi_cmd_called'.  WDYT?
>>
>> Would that be any better than just storing the name of current command
>> and check it with strcmp? Yeah, we're back to where
>> we've started. What is the problem we're trying to solve? That strcmp is
>> ugly to type and not entirely efficient?
> 
> I am adding some MI notifications, which should be suppressed.  The problem I have is that we'll have a very long 'if/else if/else if/.../' blocks to compare command name to determine which suppress flag to set.  The code smell is not good to me.  So I draft these patches to change it.


Alternatively, set the notification suppression down in the command callback itself.
I mention it for completeness.  Maybe you've considered it, and decided against it.

> 
> Ideally, we can do this in a more-OO'ed manner,
> 
>   1 add a new field 'int called' in 'struct mi_cmd',
>   2 set 'parse->cmd->called' in mi_cmd_execute to 1 and set it back to 0 when it is done.
>   3 pass 'struct mi_cmd *' to each MI command function, for example change function mi_breakpoint_created to
> 
>   mi_breakpoint_created (struct mi_cmd *self, struct breakpoint *b)
> 
>   4 inside each MI command function, return early if self->called is 1.  Then, we can get rid of mi_suppress_notification completely.

Confused.  mi_breakpoint_created is not a MI command function, but rather a
notification observer.  Whoever calls the observers (observer_notify_breakpoint_created)
is disconnected from commands, and I don't see that the coupling would be a good idea.

> 
> This will lead to more changes, so I don't implement it.  If it is acceptable to you, I can go to this way.
> 

-- 
Pedro Alves