On Thu, Jun 28, 2018 at 4:57 PM, Markus Armbruster <arm...@redhat.com> wrote: > Marc-André Lureau <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com> >> Reviewed-by: Markus Armbruster <arm...@redhat.com> >> --- >> 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