Re: [PATCH v2 18/38] qapi/events.py: Move comments into docstrings

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Sep 22, 2020 at 05:00:41PM -0400, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  scripts/qapi/events.py | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index 00a9513c16..e859fd7a52 100644
>> --- a/scripts/qapi/events.py
>> +++ b/scripts/qapi/events.py
>> @@ -52,8 +52,10 @@ def gen_event_send_decl(name: str,
>>                   proto=build_event_send_proto(name, arg_type, boxed))
>>  
>>  
>> -# Declare and initialize an object 'qapi' using parameters from 
>> build_params()
>>  def gen_param_var(typ: QAPISchemaObjectType) -> str:
>> +    """
>> +    Declare and initialize a qapi object, using parameters from 
>> `build_params`.
>
> The mention of "object 'qapi'" is gone, and this seems correct
> because there's no object called 'qapi' anywhere in this
> function.  So:
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
> Comments/questions for follow up patches, because it's beyond the
> scope of this series:
>
> - Was the doc string supposed to say "an object 'param'" instead
>   of "an object 'qapi'", as that's the name of the variable it
>   declares?

Checking history... yes.  The variable was renamed from @qapi to @param
during review.

> - The "using parameters from build_params()" part was confusing to
>   me.  I thought it meant gen_param_var() would call build_params().
>   I took a while to notice it actually meant "using the C
>   function parameters that were previously declared using
>   build_params() at build_event_send_proto()".  I don't know
>   how we could make it clearer.

What about:

    Generate a QAPI struct variable holding the event parameters,
    initialized with the function's arguments.

>> +    """
>>      assert not typ.variants
>>      ret = mcgen('''
>>      %(c_name)s param = {
>> -- 
>> 2.26.2
>>