[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] qapi: add location comment for generated
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] qapi: add location comment for generated types |
Date: |
Thu, 06 Jul 2017 17:15:41 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> Hi
>
> On Thu, Jul 6, 2017 at 10:43 AM Markus Armbruster <address@hidden> wrote:
>
>> Marc-André Lureau <address@hidden> writes:
>>
>> > This may help to find where the origin of the type was declared in the
>> > json (when greping isn't easy enough).
>> >
>> > Generates the following kind of C comment before types:
>> >
>> > /* /home/elmarco/src/qemu/qapi/introspect.json:94 */
>> > typedef struct SchemaInfo SchemaInfo;
>>
>> Can we do relative file names? I.e. just qapi/introspect.json.
>>
>
> We could, but relative to $srcdir or $builddir? I think absolute path
> avoids some potential confusion.
Source files are always relative to $srcdir.
Absolute paths are a total pain when you diff files generated in
separate trees.
>> Would such comments be useful for things other than typedefs?
>>
>
> Certainly it could, however I just needed it for typedef, didn't bother
> adding it for the rest.
Fair enough.
>> > Signed-off-by: Marc-André Lureau <address@hidden>
>> > ---
>> > scripts/qapi.py | 12 +++++++++---
>> > scripts/qapi-event.py | 2 +-
>> > scripts/qapi-types.py | 18 +++++++++---------
>> > 3 files changed, 19 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/scripts/qapi.py b/scripts/qapi.py
>> > index d37a6157e0..7f9935eec0 100644
>> > --- a/scripts/qapi.py
>> > +++ b/scripts/qapi.py
>> > @@ -1852,15 +1852,21 @@ const char *const %(c_name)s_lookup[] = {
>> > return ret
>> >
>> >
>> > -def gen_enum(name, values, prefix=None):
>> > +def build_src_loc_comment(info):
>> > + if info:
>> > + return '\n/* %s:%d */' % (info['file'], info['line'])
>>
>> Leading instead of trailing newline, hmm. Let's see how it works out.
>>
>> > + else:
>> > + return ''
>>
>> Let's drop the else: line.
>>
>
> ok
>
>
>>
>> > +
>> > +def gen_enum(info, name, values, prefix=None):
>>
>> Make that name, info, values, ... for consistency with other functions
>> taking both name and info. More of the same below.
>>
>>
> ok
>
>
>> > # append automatically generated _MAX value
>> > enum_values = values + ['_MAX']
>> >
>> > ret = mcgen('''
>> > -
>> > +%(comment)s
>>
>> Loses the blank line when not info.
>>
>> > typedef enum %(c_name)s {
>> > ''',
>> > - c_name=c_name(name))
>> > + c_name=c_name(name), comment=build_src_loc_comment(info))
>> >
>> > i = 0
>> > for value in enum_values:
>> > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
>> > index bcbef1035f..cf5e282011 100644
>> > --- a/scripts/qapi-event.py
>> > +++ b/scripts/qapi-event.py
>> > @@ -159,7 +159,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>> > self._event_names = []
>> >
>> > def visit_end(self):
>> > - self.decl += gen_enum(event_enum_name, self._event_names)
>> > + self.decl += gen_enum(None, event_enum_name, self._event_names)
>> > self.defn += gen_enum_lookup(event_enum_name, self._event_names)
>> > self._event_names = None
>> >
>> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> > index b45e7b5634..9c8a3e277b 100644
>> > --- a/scripts/qapi-types.py
>> > +++ b/scripts/qapi-types.py
>> > @@ -19,12 +19,12 @@ from qapi import *
>> > objects_seen = set()
>> >
>> >
>> > -def gen_fwd_object_or_array(name):
>> > +def gen_fwd_object_or_array(info, name):
>> > return mcgen('''
>> > -
>> > +%(comment)s
>>
>> Likewise.
>>
>> I think we should drop the newline from build_src_loc_comment()'s value,
>> and keep the blank line before its two uses.
>>
>>
> And you get an extra empty line if info is None. I don't mind.
I misread your code. The leading '\n' is kind of weird, but it works.
> Other option
> is to just add \n in the else case in build_src_loc_comment()
Could work, too. Perhaps even something like '/* built-in */\n'.
[...]