[PATCH v2 4/5] qmi-codegen: allow empty input fields
Dan Williams
dcbw at redhat.com
Thu Feb 22 16:34:18 UTC 2018
On Thu, 2018-02-22 at 06:26 +0100, Thomas Weißschuh wrote:
> 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 <aleksand
> > > er 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:
If that works, and means we don't have to use -Wxxxxx flags when
building, I guess that's OK with me.
Dan
> --- 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/2
> 018-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
> _______________________________________________
> libqmi-devel mailing list
> libqmi-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libqmi-devel
More information about the libqmi-devel
mailing list