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

[Aleksander Morgado]

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)



-- 
Aleksander
https://aleksander.es