Re: [Qemu-devel] [PATCH v6 01/15] qapi: add 'if' to top-level expressions

[Marc-André Lureau]

On Thu, Jun 28, 2018 at 4:57 PM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> Accept 'if' key in top-level elements, accepted as string or list of
>> string type. The following patches will modify the test visitor to
>> check the value is correctly saved, and generate #if/#endif code (as a
>> single #if/endif line or a series for a list).
>>
>> Example of 'if' key:
>> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
>>   'if': 'defined(TEST_IF_STRUCT)' }
>>
>> The generated code is for now *unconditional*. Later patches generate
>> the conditionals.
>>
>> A following patch for qapi-code-gen.txt will provide more complete
>> documentation for 'if' usage.
>
> This paragraph looks obsolete now, because...
>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> Reviewed-by: Markus Armbruster <address@hidden>
>> ---
>>  scripts/qapi/common.py                   | 35 ++++++++++++++++++++----
>>  tests/test-qmp-cmds.c                    |  6 ++++
>>  docs/devel/qapi-code-gen.txt             | 22 +++++++++++++++
>
> ... you update documentation right in this patch (I like that).
>
> [...]
>> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> index 88a70e4d45..7af60b48f3 100644
>> --- a/docs/devel/qapi-code-gen.txt
>> +++ b/docs/devel/qapi-code-gen.txt
>> @@ -739,6 +739,28 @@ Example: Red Hat, Inc. controls redhat.com, and may 
>> therefore add a
>>  downstream command __com.redhat_drive-mirror.
>>
>>
>> +=== Configuring the schema ===
>> +
>> +Top-level QAPI expressions.
>
> This sentence no verb :)  I suspect an editing accident ate "can take an
> 'if' key".
>
>>                               The value must be a string or a list of
>> +string.
>
> s/string/strings/
>
>>           The corresponding generated code will then guard the inclusion
>> +of that member in the larger struct or function with #if IFCOND
>> +(or several #if lines for a list), where IFCOND is the value of the
>> +'if' key.
>
> "that member" makes little sense; top-level expressions aren't members.
>
>> +
>> +'struct', 'enum', 'union', 'alternate', 'command' and 'event'
>> +top-level QAPI expressions can take an 'if' keyword like:
>> +
>> +{ 'struct': 'IfStruct', 'data': { 'foo': 'int' },
>> +  'if': 'defined(IFCOND)' }
>
> Repetitive.
>
> Moreover, I'd like to see the example spell out how conditions affect
> generated code.
>
> Here's my try:
>
> === Configuring the schema ===
>
> The 'struct', 'enum', 'union', 'alternate', 'command' and 'event'
> top-level expressions can take an 'if' key.  Its value must be a string
> or a list of strings.  A string is shorthand for a list containing just
> that string.  The code generated for the top-level expression will then
> be guarded by #if COND for each COND in the list.
>
> Example: a conditional struct
>
>  { 'struct': 'IfStruct', 'data': { 'foo': 'int' },
>    'if': ['defined(CONFIG_FOO)', 'defined(HAVE_BAR)'] }
>
> gets its generated code guarded like this:
>
>  #if defined(CONFIG_FOO)
>  #if defined(HAVE_BAR)
>  ... generated code ...
>  #endif /* defined(HAVE_BAR) */
>  #endif /* defined(CONFIG_FOO) */
>
>> +
>> +Please note that you are responsible to ensure that the C code will
>> +compile with an arbitrary combination of conditions, since the
>> +generators are unable to check it at this point.
>> +
>> +The presence of 'if' keys in the schema is reflected through to the
>> +introspection output depending on the build configuration.
>> +
>> +
>>  == Client JSON Protocol introspection ==
>>
>>  Clients of a Client JSON Protocol commonly need to figure out what
> [...]
>
> I'm happy to apply these proposals without a respin, if you like them.
>

Looks good to me,
(with 02/15 squashed)

thanks

-- 
Marc-André Lureau