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

[Dan Williams]

On Mon, 2018-02-12 at 09:37 +0000, Thomas Weißschuh wrote:
> 
> On February 12, 2018 8:58:25 AM UTC, Aleksander Morgado <aleksander at a
> leksander.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.

I wouldn't mind the slightly messier generated code if we didn't have
to use compiler flags to make it work.  If we do need to use compiler
flags, then I'm in favor of not emitting the label in the generated
code.

Would it be clearer to just define a "emit_error_out" boolean at the
top of __emit_request_creator() that gets set to true whenever a "goto
error_out" is emitted, that then controls the emission of the error_out
label at the bottom?  That way it's explicit and not depending on the
magic of "self.input.fields".

Dan