Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes: > 25.01.2022 12:07, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@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.
