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.
> 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.
?
If yes: I'm not sure about "worse". But keeping it in the parent class
feels right to me anyway, as trace events could be useful in other child
classes, too.
> 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>
> ---
> scripts/qapi/gen.py | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 995a97d2b8..def52f021e 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -192,6 +192,11 @@ def _bottom(self) -> str:
> return guardend(self.fname)
>
>
> +class QAPIGenTrace(QAPIGen):
> + def _top(self):
> + return super()._top() + '# AUTOMATICALLY GENERATED, DO NOT
> MODIFY\n\n'
> +
> +
> @contextmanager
> def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) ->
> Iterator[None]:
> """
> @@ -244,15 +249,18 @@ def __init__(self,
> what: str,
> user_blurb: str,
> builtin_blurb: Optional[str],
> - pydoc: str):
> + pydoc: str,
> + add_trace_events: bool = False):
I'd prefer naming this gen_trace_events. Happy to tweak this in my tree.
> self._prefix = prefix
> self._what = what
> self._user_blurb = user_blurb
> self._builtin_blurb = builtin_blurb
> self._pydoc = pydoc
> self._current_module: Optional[str] = None
> - self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
> + self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH,
> + Optional[QAPIGenTrace]]] = {}
> self._main_module: Optional[str] = None
> + self.add_trace_events = add_trace_events
By convention, names of private attributes start with a single _.
>
> @property
> def _genc(self) -> QAPIGenC:
> @@ -264,6 +272,14 @@ def _genh(self) -> QAPIGenH:
> assert self._current_module is not None
> return self._module[self._current_module][1]
>
> + @property
> + def _gent(self) -> QAPIGenTrace:
> + assert self.add_trace_events
> + assert self._current_module is not None
> + gent = self._module[self._current_module][2]
> + assert gent is not None
> + return gent
> +
> @staticmethod
> def _module_dirname(name: str) -> str:
> if QAPISchemaModule.is_user_module(name):
> @@ -293,7 +309,12 @@ def _add_module(self, name: str, blurb: str) -> None:
> basename = self._module_filename(self._what, name)
> genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
> genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
> - self._module[name] = (genc, genh)
> + if self.add_trace_events:
> + gent = QAPIGenTrace(basename + '.trace-events')
> + else:
> + gent = None
> +
> + self._module[name] = (genc, genh, gent)
> self._current_module = name
>
> @contextmanager
> @@ -304,11 +325,13 @@ def _temp_module(self, name: str) -> Iterator[None]:
> self._current_module = old_module
>
> def write(self, output_dir: str, opt_builtins: bool = False) -> None:
> - for name, (genc, genh) in self._module.items():
> + for name, (genc, genh, gent) in self._module.items():
> if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
> continue
> genc.write(output_dir)
> genh.write(output_dir)
> + if gent is not None:
> + gent.write(output_dir)
>
> def _begin_builtin_module(self) -> None:
> pass
I wonder whether we really need a new __init__() parameter. Could we
have ._gent() create the module on demand? This is *not* a demand.
