[PATCH v2 4/5] qmi-codegen: allow empty input fields

[Thomas Weißschuh]

On Sun, 2018-02-11T21:10+0100, Aleksander Morgado wrote:
> On Sun, Feb 11, 2018 at 7:14 PM, Thomas Weißschuh <thomas at t-8ch.de> wrote:
>> On Sun, 2018-02-11T15:20+0100, Aleksander Morgado wrote:
>>> On Fri, Feb 9, 2018 at 10:07 PM, Thomas Weißschuh <thomas at t-8ch.de> wrote:
>>>> The LOC service allows empty messages to trigger the sending of certain
>>>> indicator messages.
>>>> ---
>>>>  build-aux/qmi-codegen/Message.py | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/build-aux/qmi-codegen/Message.py b/build-aux/qmi-codegen/Message.py
>>>> index 0bf62f1..57c0d5e 100644
>>>> --- a/build-aux/qmi-codegen/Message.py
>>>> +++ b/build-aux/qmi-codegen/Message.py
>>>> @@ -179,7 +179,7 @@ class Message:
>>>>          cfile.write(
>>>>              '\n'
>>>>              '    return self;\n')
>>>> -        if self.input.fields is not None:
>>>> +        if self.input.fields:
>>>
>>> I'm confused here, why do we need the error_out code chunk if no TLVs
>>> may be given? The error_out case covers the logic when there is at
>>> least one mandatory TLV.
>>
>> The patch tries removes the label.
>> The new code could have been written as:
>>
>>         if self.input is not None and len(self.input):
>>
>> The "if len(something):" is not really idiomatic and a plain "if something:"
>> does the same (in case of a list).
>> And "if self.input is not None and self.input" is equivalent to "if self.input".
>>
> 
> Aha, understood
> 
>> Long story, short: We only need the label if we can jump to it.
>>
> 
> Yes we agree on that
> 
>>> Also, wouldn't this be equivalent to just removing the if? I'm
>>> confused and not a python dev :) What I do see is that the error_out
>>> case is needed only if the logic previously added a goto error_out,
>>> which only happens a bit above in that code in another "if
>>> self.input.fields is not None:" check done. That means that removing
>>> the "is not None" from the if will actually not change the code at all
>>> (as there is no real goto error_out being added).
>>
>> We need the if-block was the compiler will error out if we define a jump label
>> but not use it.
>>
>> Actually I am now in favour of getting rid of "-Werror=unused-label".
>> What is your opinion?
>>
> 
> I believe we agree on how it should look like: no label defined and no
> goto label OR label defined and goto label. If I'm not reading it
> wrong, that was what the code was doing before this change, as both
> the label and goto label were being defined over the same if condition
> ( if self.input.fields is not None). But after the change one if()
> condition and the other are different.
> 
> How about changing both to "if self.input.fields"? (i.e. instead of
> one still saying if self.input.fields is not None)

My current version now just always adds the label but marks it with
"G_GNUC_UNUSED". This way we can keep all the logic in one place.

This is actually the exact usecase for the "unused" attribute on labels[0]:

unused
	This feature is intended for program-generated code that may contain
	unused labels, but which is compiled with -Wall. It is not normally
	appropriate to use in it human-written code, though it could be useful
	in cases where the code that jumps to the label is contained within an
	#ifdef conditional.

What do you think?


[0] https://gcc.gnu.org/onlinedocs/gcc/Label-Attributes.html

Thomas