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

[Thomas Weißschuh]

On February 12, 2018 8:58:25 AM UTC, Aleksander Morgado <aleksander at aleksander.es> wrote:
>On Sun, Feb 11, 2018 at 9:54 PM, Thomas Weißschuh <thomas at t-8ch.de>
>wrote:
>
>> 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?
>>
>>
>​Don't know, I think I would personally still prefer to not issue the
>label
>if it's not going to be needed. What is the problem with doing that?

We would have to duplicate the logic which is split over nested if-clauses.
Personally I prefer the cleaner code in the generator over cleaner generated code.

Your choice though.

Thomas