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

[Aleksander Morgado]

On 12/02/18 10:37, Thomas Weißschuh wrote:
> 
> 
> 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.
> 

For some reason I've always written the code generator thinking in having readable clean code, I didn't mind making the generator more complex as long as the generated code looked easier to read and understand. I'm not sure I like having some unused labels around, even if marked as unused to avoid the warning... Don't know what others think about this? Dan, any opinion?

-- 
Aleksander
https://aleksander.es