Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Feb 20, 2023 at 12:10 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Hi Markus
>> >
>> > On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <armbru@redhat.com>
>> > wrote:
>> >
>> >> marcandre.lureau@redhat.com writes:
>> >>
>> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> >
>> >> > The generated code doesn't quite handle the conditional arguments.
>> >> > For example, 'bar' in 'test-if-cmd' is not correctly surrounded by #if
>> >> > conditions. See generated code in qmp_marshal_test_if_cmd().
>> >> >
>> >> > Note that if there are multiple optional arguments at the last position,
>> >> > there might be compilation issues due to extra comas. I left an assert
>> >> > and FIXME for later.
>> >> >
>> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> > ---
>> >> >  scripts/qapi/commands.py                |  4 ++++
>> >> >  scripts/qapi/gen.py                     | 19 ++++++++++++++-----
>> >> >  scripts/qapi/visit.py                   |  2 ++
>> >> >  tests/qapi-schema/qapi-schema-test.json |  3 ++-
>> >> >  4 files changed, 22 insertions(+), 6 deletions(-)
>> >> >
>> >> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> >> > index 79c5e5c3a9..07997d1586 100644
>> >> > --- a/scripts/qapi/commands.py
>> >> > +++ b/scripts/qapi/commands.py
>> >> > @@ -64,9 +64,13 @@ def gen_call(name: str,
>> >> >      elif arg_type:
>> >> >          assert not arg_type.variants
>> >> >          for memb in arg_type.members:
>> >> > +            if memb.ifcond.is_present():
>> >> > +                argstr += '\n' + memb.ifcond.gen_if()
>> >> >              if memb.need_has():
>> >> >                  argstr += 'arg.has_%s, ' % c_name(memb.name)
>> >> >              argstr += 'arg.%s, ' % c_name(memb.name)
>> >> > +            if memb.ifcond.is_present():
>> >> > +                argstr += '\n' + memb.ifcond.gen_endif()
>> >> >
>> >> >      lhs = ''
>> >> >      if ret_type:
>> >>
>> >> @argstr is emitted further down:
>> >>
>> >>        %(lhs)sqmp_%(name)s(%(args)s&err);
>> >>    ''',
>> >>                     name=name, args=argstr, lhs=lhs)
>> >>
>> >>        ret += mcgen('''
>> >>        if (err) {
>> >>    ''')
>> >>
>> >> Before the patch, @argstr contains no newlines.  Works.
>> >>
>> >> After the patch, it may contain newlines, and if it does, intentation is
>> >> messed up.  For instance, in the code generated for
>> >> qapi-schema-test.json:
>> >>
>> >>         retval = qmp_test_if_cmd(arg.foo,
>> >>     #if defined(TEST_IF_CMD_BAR)
>> >>     arg.bar,
>> >>     #endif /* defined(TEST_IF_CMD_BAR) */
>> >>     &err);
>> >>
>> >> Strings interpolated into the mcgen() argument should not contain
>> >> newlines.  I'm afraid you have to rewrite the code emitting the call.
>> >>
>> >
>> > Why it should not contain newlines?
>>
>> They mess up indentation.  I think.  It's been a while...  All I really
>> know for sure is that the generated code's indentation is messed up
>> right there.
>>
>> > What are you asking exactly? that the caller be changed? (this does not
>> > work well if there are multiple optional arguments..)
>> >
>> >     #if defined(TEST_IF_CMD_BAR)
>> >         retval = qmp_test_if_cmd(arg.foo, arg.bar, &err);
>> >     #else
>> >         retval = qmp_test_if_cmd(arg.foo, &err);
>> >     #endif /* defined(TEST_IF_CMD_BAR) */
>>
>> I'm asking for better indentation.  In handwritten code, we'd do
>>
>>         retval = qmp_test_if_cmd(arg.foo,
>>     #if defined(TEST_IF_CMD_BAR)
>>                                  arg.bar,
>>     #endif /* defined(TEST_IF_CMD_BAR) */
>>                                  &err);
>>
>> Keeping track of how far to indent the arguments is bothersome in the
>> generator, though.  Perhaps we could create infrastructure to make it
>> not bothersome, but I'm not asking for that.  Something like this should
>> be good enough:
>>
>>         retval = qmp_test_if_cmd(arg.foo,
>>     #if defined(TEST_IF_CMD_BAR)
>>                     arg.bar,
>>     #endif /* defined(TEST_IF_CMD_BAR) */
>>                     &err);
>>
>> I.e. indent to the function call and then some.
>
> ok, I improved the indentation a bit.
>
> However, I think it would be simpler, and better, if we piped the
> generated code to clang-format (when available). I made a simple patch
> for that too.

Piping through indent or clang-format may well give us neater results
for less effort.

We might want to dumb down generator code then.

>> >> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> >> > index b5a8d03e8e..ba57e72c9b 100644
>> >> > --- a/scripts/qapi/gen.py
>> >> > +++ b/scripts/qapi/gen.py
>> >> > @@ -111,22 +111,31 @@ def build_params(arg_type: 
>> >> > Optional[QAPISchemaObjectType],
>> >> >                   boxed: bool,
>> >> >                   extra: Optional[str] = None) -> str:
>> >> >      ret = ''
>> >> > -    sep = ''
>> >> >      if boxed:
>> >> >          assert arg_type
>> >> >          ret += '%s arg' % arg_type.c_param_type()
>> >> > -        sep = ', '
>> >> > +        if extra:
>> >> > +            ret += ', '
>> >> >      elif arg_type:
>> >> >          assert not arg_type.variants
>> >> > +        n = 0
>> >> >          for memb in arg_type.members:
>> >> > -            ret += sep
>> >> > -            sep = ', '
>> >> > +            n += 1
>> >> > +            if memb.ifcond.is_present():
>> >> > +                ret += '\n' + memb.ifcond.gen_if()
>> >> >              if memb.need_has():
>> >> >                  ret += 'bool has_%s, ' % c_name(memb.name)
>> >> >              ret += '%s %s' % (memb.type.c_param_type(),
>> >> >                                c_name(memb.name))
>> >> > +            if extra or n != len(arg_type.members):
>> >> > +                ret += ', '
>> >> > +            else:
>> >> > +                # FIXME: optional last argument may break compilation
>> >> > +                assert not memb.ifcond.is_present()
>> >>
>> >> Does the assertion guard against the C compilation failure?
>> >
>> > Yes
>> >
>> >>
>> >> Is it possible to write schema code that triggers it?
>> >
>> > Yes, the one we have for TEST_IF_EVENT for example:
>> >
>> > { 'event': 'TEST_IF_EVENT',
>> >   'data': { 'foo': 'TestIfStruct',
>> >             'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },
>>
>> This is the one you put in qapi-schema-test.json less the last
>> parameter, so that the conditional parameter becomes the last one.
>>
>> > produces:
>> >
>> > void qapi_event_send_test_if_event(TestIfStruct *foo,
>> > #if defined(TEST_IF_EVT_BAR)
>> > TestIfEnumList *bar,
>> > #endif /* defined(TEST_IF_EVT_BAR) */
>> > );
>> >
>> > Which will fail to compile if TEST_IF_EVT_BAR is undefined.
>>
>> I think it'll fail to compile always, because the parameter list has a
>> trailing comma regardless of TEST_IF_EVT_BAR.
>
> Yes, I think I hand-wrote that example, the actual generator does not
> leave a trailing comma here.
>
>>
>> > So I would rather assert that we don't introduce such a schema, until we
>> > fix the code generator. Or we acknowledge the limitation, and treat it as a
>> > schema error. Other ideas?
>>
>> Yes: throw an error.  Assertions are for programming errors.  This isn't
>> a programming error, it's a limitation of the current implementation.
>>
>> How hard would it be to lift the limitation?
>
> Taking this as a problematic example:
>
> void function(first,
> #ifdef A
>     a,
> #endif
> #ifdef B
>     b
> #endif
> )
>
> I think it would mean that we would have to pass arguments as a
> structure, as they don't have the limitation of trailing coma in
> initializers. That would not be idiomatic C though, and we would need
> to refactor a lot of code..
>
> Another option is to always pass a dummy last argument? :)
>
> void command(first,
> #ifdef A
>     a,
> #endif
> #ifdef B
>     b,
> #endif
>     dummy)

Yet another option:

  void command(first
  #ifdef A
      , a
  #endif
  #ifdef B
      , b
  #endif
      )

[...]