Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Mon, Feb 20, 2023 at 12:10 PM Markus Armbruster <arm...@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>>
>> > Hi Markus
>> >
>> > On Fri, Feb 17, 2023 at 12:28 PM Markus Armbruster <arm...@redhat.com>
>> > wrote:
>> >
>> >> marcandre.lur...@redhat.com writes:
>> >>
>> >> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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
)
[...]
