[PATCH v2 4/5] qmi-codegen: allow empty input fields
Thomas Weißschuh
thomas at t-8ch.de
Thu Feb 22 05:26:22 UTC 2018
On Wed, 2018-02-21T16:39-0600, Dan Williams wrote:
> Date: Wed, 21 Feb 2018 16:39:12 -0600
> From: Dan Williams <dcbw at redhat.com>
> To: Thomas Weißschuh <thomas at t-8ch.de>, Aleksander Morgado
> <aleksander at aleksander.es>
> Cc: "libqmi (development)" <libqmi-devel at lists.freedesktop.org>
> Subject: Re: [PATCH v2 4/5] qmi-codegen: allow empty input fields
>
> 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.
What do you think about the G_GNUC_UNUSED attribute?
My current (complete) patch is below, removing all conditionals:
--- a/build-aux/qmi-codegen/Message.py
+++ b/build-aux/qmi-codegen/Message.py
@@ -178,14 +178,12 @@ class Message:
' }\n')
cfile.write(
'\n'
- ' return self;\n')
- if self.input.fields is not None:
- cfile.write(
- '\n'
- 'error_out:\n'
- ' qmi_message_unref (self);\n'
- ' return NULL;\n')
- cfile.write(
+ ' return self;\n'
+ '\n'
+ 'error_out:\n'
+ 'G_GNUC_UNUSED;\n'
+ ' qmi_message_unref (self);\n'
+ ' return NULL;\n'
'}\n')
>
> 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".
One problem is, that the error_out is actually only used from
Field.emit_input_tlv_add().
The code in Message.py will never emit error_out because it checks if
field.manatory == 'yes' while field.mandatory is only True or False.
I fixed this in https://lists.freedesktop.org/archives/libqmi-devel/2018-February/002745.html,
which should probably have been two patches
Also error_out depends on the types of fields actually in the input.
VariableInteger and VariableString (in addition to Message and Field) use it
will for example VariableArray, VariableSequence and VariableStruct don't.
Having a container without any primitive type in it does probably not make much
sense, but maybe there are some valid usecases.
Regards,
Thomas
More information about the libqmi-devel
mailing list