Re: [PATCH 3/3] suppress notification

Pedro Alves
2012-08-28T13:40:15Z

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