Re: [PATCH 06/37] qapi: delint using flake8
On 9/21/20 3:31 AM, Markus Armbruster wrote: John Snow writes: On 9/18/20 6:33 AM, Markus Armbruster wrote: John Snow writes: [...] 2. Writing a templating engine that doesn't break the Python indentation flow by hoisting them into module constants and improving the rendering logic. Too vague for me to judge, happy to look at PoC patches. Okay, some other day. Not important enough presently. It's just hard to stop brainstorming ideas as I learn about and touch different areas of code. --js
Re: [PATCH 06/37] qapi: delint using flake8
John Snow writes: > On 9/18/20 6:33 AM, Markus Armbruster wrote: >> John Snow writes: [...] >>> Embedded templates are always gonna look kinda nasty, I think, because >>> you're trying to fight style guidelines in two languages >>> simultaneously and it's never gonna quite work out exactly how you >>> want without some pretty complex abstraction mechanisms that are well >>> beyond the power we need right now. >> The thought "screw this, pile the output through /usr/bin/indent" >> has >> crossed my mind more than once. >> > > Bigger fish to fry, but with other languages like rust looming, making > the core generation facilities nicer might be ...nice. Not for this > series. Definitely not. The way we generate C now is (close to) the stupidest way that could possibly work. Stupid is *good*, until it creates actual problems. Any improvement will have to balance readability vs. hackability. What we have now is often hard to read, but pretty easy to hack. May sound like a contradiction, but it's been my experience. > Two approaches in general would make sense to me: > > 1. Building an AST for C instead of strings, and rendering the AST. I fear the need to define an AST for C makes this uneconomical. I've missed the ease of generating Lisp in Lisp many times. > 2. Writing a templating engine that doesn't break the Python > indentation flow by hoisting them into module constants and improving > the rendering logic. Too vague for me to judge, happy to look at PoC patches.
Re: [PATCH 06/37] qapi: delint using flake8
On 9/18/20 6:33 AM, Markus Armbruster wrote:
John Snow writes:
On 9/17/20 3:54 AM, Markus Armbruster wrote:
John Snow writes:
On 9/16/20 8:12 AM, Markus Armbruster wrote:
John Snow writes:
Petty style guide fixes and line length enforcement. Not a big win, not
a big loss, but flake8 passes 100% on the qapi module, which gives us an
easy baseline to enforce hereafter.
Signed-off-by: John Snow
[...]
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index e1df0e341f..2e4b4de0fa 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -69,7 +69,8 @@ def gen_call(name, arg_type, boxed, ret_type):
def gen_marshal_output(ret_type):
return mcgen('''
-static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
QObject **ret_out, Error **errp)
+static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
+ Error **errp)
The continued parameter list may become misalignd in generated C.
E.g.
static void qmp_marshal_output_BlockInfoList(BlockInfoList *ret_in,
QObject **ret_out,
Error **errp)
{
...
}
Do we care?
Yeah, I don't know. Do we?
I care, but I also care for automated style checks.
It actually seemed more annoying to try and get flake8 to make an
exception for these handful of examples.
Path of least resistance led me here, but I can try and appease both
systems if you'd prefer.
Up to now, I ran the style checkers manually, and this was just one
of
several complaints to ignore, so I left the code alone.
If it gets in the way of running them automatically, and messing up
the
generated code slightly is the easiest way to get it out of the way,
then I can accept the slight mess.
I changed this a little to put all the args on the next line, which is
slightly unusual but works okay.
I think it's slightly more unusual than the non-matching indentation
was.
Yet another way to skin this cat:
static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
QObject **ret_out, Error **errp)
Now the second line is not aligned with the left parenthesis both in the
Python source and in the generated C.
Your wish is my command!
(Except in the other replies where I am arguing with you.)
I think that's a fine middle ground, because the alternative (to me)
is to start using abstracted code generation tokens in a tree
structure, etc etc etc.
Embedded templates are always gonna look kinda nasty, I think, because
you're trying to fight style guidelines in two languages
simultaneously and it's never gonna quite work out exactly how you
want without some pretty complex abstraction mechanisms that are well
beyond the power we need right now.
The thought "screw this, pile the output through /usr/bin/indent" has
crossed my mind more than once.
Bigger fish to fry, but with other languages like rust looming, making
the core generation facilities nicer might be ...nice. Not for this series.
Two approaches in general would make sense to me:
1. Building an AST for C instead of strings, and rendering the AST.
2. Writing a templating engine that doesn't break the Python indentation
flow by hoisting them into module constants and improving the rendering
logic.
--js
Re: [PATCH 06/37] qapi: delint using flake8
John Snow writes:
> On 9/17/20 3:54 AM, Markus Armbruster wrote:
>> John Snow writes:
>>
>>> On 9/16/20 8:12 AM, Markus Armbruster wrote:
John Snow writes:
> Petty style guide fixes and line length enforcement. Not a big win, not
> a big loss, but flake8 passes 100% on the qapi module, which gives us an
> easy baseline to enforce hereafter.
>
> Signed-off-by: John Snow
[...]
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index e1df0e341f..2e4b4de0fa 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -69,7 +69,8 @@ def gen_call(name, arg_type, boxed, ret_type):
>def gen_marshal_output(ret_type):
>return mcgen('''
>-static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
> QObject **ret_out, Error **errp)
> +static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject
> **ret_out,
> + Error **errp)
The continued parameter list may become misalignd in generated C.
E.g.
static void qmp_marshal_output_BlockInfoList(BlockInfoList *ret_in,
QObject **ret_out,
Error **errp)
{
...
}
Do we care?
>>>
>>> Yeah, I don't know. Do we?
>> I care, but I also care for automated style checks.
>>
>>> It actually seemed more annoying to try and get flake8 to make an
>>> exception for these handful of examples.
>>>
>>> Path of least resistance led me here, but I can try and appease both
>>> systems if you'd prefer.
>> Up to now, I ran the style checkers manually, and this was just one
>> of
>> several complaints to ignore, so I left the code alone.
>> If it gets in the way of running them automatically, and messing up
>> the
>> generated code slightly is the easiest way to get it out of the way,
>> then I can accept the slight mess.
>>
>
> I changed this a little to put all the args on the next line, which is
> slightly unusual but works okay.
I think it's slightly more unusual than the non-matching indentation
was.
Yet another way to skin this cat:
static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
QObject **ret_out, Error **errp)
Now the second line is not aligned with the left parenthesis both in the
Python source and in the generated C.
> I think that's a fine middle ground, because the alternative (to me)
> is to start using abstracted code generation tokens in a tree
> structure, etc etc etc.
>
> Embedded templates are always gonna look kinda nasty, I think, because
> you're trying to fight style guidelines in two languages
> simultaneously and it's never gonna quite work out exactly how you
> want without some pretty complex abstraction mechanisms that are well
> beyond the power we need right now.
The thought "screw this, pile the output through /usr/bin/indent" has
crossed my mind more than once.
Re: [PATCH 06/37] qapi: delint using flake8
On 9/17/20 3:54 AM, Markus Armbruster wrote:
John Snow writes:
On 9/16/20 8:12 AM, Markus Armbruster wrote:
John Snow writes:
Petty style guide fixes and line length enforcement. Not a big win, not
a big loss, but flake8 passes 100% on the qapi module, which gives us an
easy baseline to enforce hereafter.
Signed-off-by: John Snow
---
scripts/qapi/.flake8 | 2 ++
scripts/qapi/commands.py | 3 ++-
scripts/qapi/schema.py | 8 +---
scripts/qapi/visit.py| 15 ++-
4 files changed, 19 insertions(+), 9 deletions(-)
create mode 100644 scripts/qapi/.flake8
diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
new file mode 100644
index 00..45d8146f3f
--- /dev/null
+++ b/scripts/qapi/.flake8
@@ -0,0 +1,2 @@
+[flake8]
+extend-ignore = E722 # Pylint handles this, but smarter.
I guess you mean pylint's W0702 a.k.a. bare-except. What's wrong
with
flake8's E722 compared to pylint's W0702?
Flake8 will warn on *any* bare except, but Pylint's is context-aware
and will suppress the warning if you re-raise the exception.
Should this information be worked into the comment?
I'll improve it a little, but I'll add what I wrote above to the commit
message.
I don't actually think this comes up in the qapi code base, but it
does come up in the ./python/qemu code base.
(One of my goals is unifying the lint checking regime for both packages.)
\ No newline at end of file
So put one there :)
Whupps, okay.
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index e1df0e341f..2e4b4de0fa 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -69,7 +69,8 @@ def gen_call(name, arg_type, boxed, ret_type):
def gen_marshal_output(ret_type):
return mcgen('''
-static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
QObject **ret_out, Error **errp)
+static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
+ Error **errp)
The continued parameter list may become misalignd in generated C.
E.g.
static void qmp_marshal_output_BlockInfoList(BlockInfoList
*ret_in, QObject **ret_out,
Error **errp)
{
...
}
Do we care?
Yeah, I don't know. Do we?
I care, but I also care for automated style checks.
It actually seemed more annoying to try and get flake8 to make an
exception for these handful of examples.
Path of least resistance led me here, but I can try and appease both
systems if you'd prefer.
Up to now, I ran the style checkers manually, and this was just one of
several complaints to ignore, so I left the code alone.
If it gets in the way of running them automatically, and messing up the
generated code slightly is the easiest way to get it out of the way,
then I can accept the slight mess.
I changed this a little to put all the args on the next line, which is
slightly unusual but works okay.
I think that's a fine middle ground, because the alternative (to me) is
to start using abstracted code generation tokens in a tree structure,
etc etc etc.
Embedded templates are always gonna look kinda nasty, I think, because
you're trying to fight style guidelines in two languages simultaneously
and it's never gonna quite work out exactly how you want without some
pretty complex abstraction mechanisms that are well beyond the power we
need right now.
More of the same below.
{
Visitor *v;
[...]
Re: [PATCH 06/37] qapi: delint using flake8
John Snow writes:
> On 9/16/20 8:12 AM, Markus Armbruster wrote:
>> John Snow writes:
>>
>>> Petty style guide fixes and line length enforcement. Not a big win, not
>>> a big loss, but flake8 passes 100% on the qapi module, which gives us an
>>> easy baseline to enforce hereafter.
>>>
>>> Signed-off-by: John Snow
>>> ---
>>> scripts/qapi/.flake8 | 2 ++
>>> scripts/qapi/commands.py | 3 ++-
>>> scripts/qapi/schema.py | 8 +---
>>> scripts/qapi/visit.py| 15 ++-
>>> 4 files changed, 19 insertions(+), 9 deletions(-)
>>> create mode 100644 scripts/qapi/.flake8
>>>
>>> diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
>>> new file mode 100644
>>> index 00..45d8146f3f
>>> --- /dev/null
>>> +++ b/scripts/qapi/.flake8
>>> @@ -0,0 +1,2 @@
>>> +[flake8]
>>> +extend-ignore = E722 # Pylint handles this, but smarter.
>> I guess you mean pylint's W0702 a.k.a. bare-except. What's wrong
>> with
>> flake8's E722 compared to pylint's W0702?
>>
>
> Flake8 will warn on *any* bare except, but Pylint's is context-aware
> and will suppress the warning if you re-raise the exception.
Should this information be worked into the comment?
> I don't actually think this comes up in the qapi code base, but it
> does come up in the ./python/qemu code base.
>
> (One of my goals is unifying the lint checking regime for both packages.)
>
>>> \ No newline at end of file
>> So put one there :)
>>
>
> Whupps, okay.
>
>>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>>> index e1df0e341f..2e4b4de0fa 100644
>>> --- a/scripts/qapi/commands.py
>>> +++ b/scripts/qapi/commands.py
>>> @@ -69,7 +69,8 @@ def gen_call(name, arg_type, boxed, ret_type):
>>> def gen_marshal_output(ret_type):
>>> return mcgen('''
>>> -static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>>> QObject **ret_out, Error **errp)
>>> +static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject
>>> **ret_out,
>>> + Error **errp)
>> The continued parameter list may become misalignd in generated C.
>> E.g.
>> static void qmp_marshal_output_BlockInfoList(BlockInfoList
>> *ret_in, QObject **ret_out,
>>Error **errp)
>> {
>> ...
>> }
>> Do we care?
>>
>
> Yeah, I don't know. Do we?
I care, but I also care for automated style checks.
> It actually seemed more annoying to try and get flake8 to make an
> exception for these handful of examples.
>
> Path of least resistance led me here, but I can try and appease both
> systems if you'd prefer.
Up to now, I ran the style checkers manually, and this was just one of
several complaints to ignore, so I left the code alone.
If it gets in the way of running them automatically, and messing up the
generated code slightly is the easiest way to get it out of the way,
then I can accept the slight mess.
>> More of the same below.
>>
>>> {
>>> Visitor *v;
>>>
>> [...]
>>
Re: [PATCH 06/37] qapi: delint using flake8
On 9/16/20 8:12 AM, Markus Armbruster wrote:
John Snow writes:
Petty style guide fixes and line length enforcement. Not a big win, not
a big loss, but flake8 passes 100% on the qapi module, which gives us an
easy baseline to enforce hereafter.
Signed-off-by: John Snow
---
scripts/qapi/.flake8 | 2 ++
scripts/qapi/commands.py | 3 ++-
scripts/qapi/schema.py | 8 +---
scripts/qapi/visit.py| 15 ++-
4 files changed, 19 insertions(+), 9 deletions(-)
create mode 100644 scripts/qapi/.flake8
diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
new file mode 100644
index 00..45d8146f3f
--- /dev/null
+++ b/scripts/qapi/.flake8
@@ -0,0 +1,2 @@
+[flake8]
+extend-ignore = E722 # Pylint handles this, but smarter.
I guess you mean pylint's W0702 a.k.a. bare-except. What's wrong with
flake8's E722 compared to pylint's W0702?
Flake8 will warn on *any* bare except, but Pylint's is context-aware and
will suppress the warning if you re-raise the exception.
I don't actually think this comes up in the qapi code base, but it does
come up in the ./python/qemu code base.
(One of my goals is unifying the lint checking regime for both packages.)
\ No newline at end of file
So put one there :)
Whupps, okay.
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index e1df0e341f..2e4b4de0fa 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -69,7 +69,8 @@ def gen_call(name, arg_type, boxed, ret_type):
def gen_marshal_output(ret_type):
return mcgen('''
-static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out, Error **errp)
+static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
+ Error **errp)
The continued parameter list may become misalignd in generated C. E.g.
static void qmp_marshal_output_BlockInfoList(BlockInfoList *ret_in,
QObject **ret_out,
Error **errp)
{
...
}
Do we care?
Yeah, I don't know. Do we?
It actually seemed more annoying to try and get flake8 to make an
exception for these handful of examples.
Path of least resistance led me here, but I can try and appease both
systems if you'd prefer.
More of the same below.
{
Visitor *v;
[...]
Re: [PATCH 06/37] qapi: delint using flake8
John Snow writes:
> Petty style guide fixes and line length enforcement. Not a big win, not
> a big loss, but flake8 passes 100% on the qapi module, which gives us an
> easy baseline to enforce hereafter.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/.flake8 | 2 ++
> scripts/qapi/commands.py | 3 ++-
> scripts/qapi/schema.py | 8 +---
> scripts/qapi/visit.py| 15 ++-
> 4 files changed, 19 insertions(+), 9 deletions(-)
> create mode 100644 scripts/qapi/.flake8
>
> diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
> new file mode 100644
> index 00..45d8146f3f
> --- /dev/null
> +++ b/scripts/qapi/.flake8
> @@ -0,0 +1,2 @@
> +[flake8]
> +extend-ignore = E722 # Pylint handles this, but smarter.
I guess you mean pylint's W0702 a.k.a. bare-except. What's wrong with
flake8's E722 compared to pylint's W0702?
> \ No newline at end of file
So put one there :)
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index e1df0e341f..2e4b4de0fa 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -69,7 +69,8 @@ def gen_call(name, arg_type, boxed, ret_type):
> def gen_marshal_output(ret_type):
> return mcgen('''
>
> -static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject
> **ret_out, Error **errp)
> +static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject
> **ret_out,
> + Error **errp)
The continued parameter list may become misalignd in generated C. E.g.
static void qmp_marshal_output_BlockInfoList(BlockInfoList *ret_in, QObject
**ret_out,
Error **errp)
{
...
}
Do we care?
More of the same below.
> {
> Visitor *v;
>
[...]
[PATCH 06/37] qapi: delint using flake8
Petty style guide fixes and line length enforcement. Not a big win, not
a big loss, but flake8 passes 100% on the qapi module, which gives us an
easy baseline to enforce hereafter.
Signed-off-by: John Snow
---
scripts/qapi/.flake8 | 2 ++
scripts/qapi/commands.py | 3 ++-
scripts/qapi/schema.py | 8 +---
scripts/qapi/visit.py| 15 ++-
4 files changed, 19 insertions(+), 9 deletions(-)
create mode 100644 scripts/qapi/.flake8
diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
new file mode 100644
index 00..45d8146f3f
--- /dev/null
+++ b/scripts/qapi/.flake8
@@ -0,0 +1,2 @@
+[flake8]
+extend-ignore = E722 # Pylint handles this, but smarter.
\ No newline at end of file
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index e1df0e341f..2e4b4de0fa 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -69,7 +69,8 @@ def gen_call(name, arg_type, boxed, ret_type):
def gen_marshal_output(ret_type):
return mcgen('''
-static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject
**ret_out, Error **errp)
+static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
+ Error **errp)
{
Visitor *v;
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index a835ee6fde..b77e0e56b2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -536,7 +536,7 @@ def set_defined_in(self, name):
v.set_defined_in(name)
def check(self, schema, seen):
-if not self.tag_member: # flat union
+if not self.tag_member: # flat union
self.tag_member = seen.get(c_name(self._tag_name))
base = "'base'"
# Pointing to the base type when not implicit would be
@@ -821,7 +821,7 @@ def __init__(self, fname):
self._entity_dict = {}
self._module_dict = OrderedDict()
self._schema_dir = os.path.dirname(fname)
-self._make_module(None) # built-ins
+self._make_module(None) # built-ins
self._make_module(fname)
self._predefining = True
self._def_predefineds()
@@ -965,7 +965,9 @@ def _make_implicit_object_type(self, name, info, ifcond,
role, members):
# But it's not tight: the disjunction need not imply it. We
# may end up compiling useless wrapper types.
# TODO kill simple unions or implement the disjunction
-assert (ifcond or []) == typ._ifcond # pylint:
disable=protected-access
+
+# pylint: disable=protected-access
+assert (ifcond or []) == typ._ifcond
else:
self._def_entity(QAPISchemaObjectType(
name, info, None, ifcond, None, None, members, None))
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index ea277e7704..31bf46e7f7 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -31,7 +31,8 @@ def gen_visit_decl(name, scalar=False):
if not scalar:
c_type += '*'
return mcgen('''
-bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error
**errp);
+bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj,
+ Error **errp);
''',
c_name=c_name(name), c_type=c_type)
@@ -125,7 +126,8 @@ def gen_visit_object_members(name, base, members, variants):
def gen_visit_list(name, element_type):
return mcgen('''
-bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
Error **errp)
+bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
+ Error **errp)
{
bool ok = false;
%(c_name)s *tail;
@@ -158,7 +160,8 @@ def gen_visit_list(name, element_type):
def gen_visit_enum(name):
return mcgen('''
-bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj,
Error **errp)
+bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj,
+ Error **errp)
{
int value = *obj;
bool ok = visit_type_enum(v, name, &value, &%(c_name)s_lookup, errp);
@@ -172,7 +175,8 @@ def gen_visit_enum(name):
def gen_visit_alternate(name, variants):
ret = mcgen('''
-bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
Error **errp)
+bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
+ Error **errp)
{
bool ok = false;
@@ -247,7 +251,8 @@ def gen_visit_alternate(name, variants):
def gen_visit_object(name, base, members, variants):
return mcgen('''
-bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
Error **errp)
+bool visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
+ Error **errp)
{
bool ok = false;
--
2.26.2
