Re: [PATCH v4 1/3] scripts/qapi/gen.py: add FOO.trace-events output module

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 25.01.2022 12:07, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> We are going to generate trace events for qmp commands. We should
>> 
>> QMP
>> 
>>> generate both trace_*() function calls and trace-events files listing
>>> events for trace generator.
>>>
>>> So, add an output module FOO.trace-events for each FOO schema module.
>>>
>>> Still, we'll need these .trace-events files only for
>>> QAPISchemaGenCommandVisitor successor of QAPISchemaModularCVisitor.
>>> So, make this possibility optional, to avoid generating extra empty
>>> files for all other successors of QAPISchemaModularCVisitor.
>> 
>> Suggest to make this slightly less technical for easier reading:
>> 
>>    Since we're going to add trace events only to command marshallers,
>>    make the trace-events output optional, so we don't generate so many
>>    useless empty files.
>
> Sounds good
>
>> 
>>> We can't simply add the new feature directly to
>>> QAPISchemaGenCommandVisitor: this means we'll have to reimplement
>>> a kind of ._module / .write() functionality of parent class in the
>>> successor, which seems worse than extending base class functionality.
>> 
>> Do you mean something like
>> 
>>    The alternative would be adding the the new feature directly to
>>    QAPISchemaGenCommandVisitor, but then we'd have to reimplement the
>>    ._module / .write() functionality of its parent class
>>    QAPISchemaModularCVisitor, which seems worse than extending the parent
>>    class.
>> 
>> ?
>
> Yes.
>
>> 
>> If yes: I'm not sure about "worse". 
>
> Hmm. *shrug* ) I'm new to this code, that's why it seems unobvious to me, and 
> that's why I'm afraid of deeper refactoring)
>
>> But keeping it in the parent class
>> feels right to me anyway, as trace events could be useful in other child
>> classes, too.
>
> If it is OK, we can simply drop this paragraph.

Works for me.

Keeping it would work, too.  "Seems worse" is an opinion, not wrong.

>>> Currently nobody set add_trace_events to True, so new functionality is
>>> formally disabled. It will be enabled for QAPISchemaGenCommandVisitor
>> 
>> Drop "formally".
>> 
>>> in further commit.
>> 
>> "in a further commit", or "in the next commit".
>> 
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

[...]

>> I wonder whether we really need a new __init__() parameter.  Could we
>> have ._gent() create the module on demand?  This is *not* a demand.
>> 
>
> My first attempt to drop extra empty generated .trace-events files was to 
> teach QAPIGenTrace() not to generate file when it is empty. But in this case 
> some empty .trace-events for commands are not generated, and "include" line 
> fails to compile. And at time when include line is generated, I don't know 
> will corresponding .trace-events be empty or not. So I decided to make a new 
> parameter for __init__()

Okay.  We can always improve on top if we care.