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

[Aleksander Morgado]

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?

-- 
Aleksander
https://aleksander.es
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/libqmi-devel/attachments/20180212/4d517c1c/attachment.html>