style suggestions for that whole "_append" and leading space thing

style suggestions for that whole "_append" and leading space thing

[Robert P. J. Day]

  getting pedantic (as i am wont to do), occasionally i run across
things like this, from meta/conf/distro/include/no-static-libs.inc:

  EXTRA_OECONF_append = "${DISABLE_STATIC}"

which, at first glance, simply seems wrong given the lack of a leading
space, until one looks higher up in that same file to read:

  DISABLE_STATIC = " --disable-static"

where one finds the leading space as part of the variable itself. i
find this potentially *really* confusing; consistent style suggests
variables should be assigned their values without regard as to how
they might be used later. subsequent references or usages of those
variables should be responsible for making sure they're included
properly, no?

  and the last line in that same file also suggests potential misuse:

  EXTRA_OECMAKE_append_pn-libical = "-DSHARED_ONLY=True"

rday

p.s. would the same logic hold with lines like this?

meta/conf/machine/include/tune-ppce6500.inc:

  MACHINE_FEATURES_BACKFILL_CONSIDERED_append =
     "${@bb.utils.contains('TUNE_FEATURES', 'e6500', ' qemu-usermode', '', d)}"

would it not be clearer if one were to write:

  MACHINE_FEATURES_BACKFILL_CONSIDERED_append =
     " ${@bb.utils.contains('TUNE_FEATURES', 'e6500', 'qemu-usermode', '', d)}"

Re: style suggestions for that whole "_append" and leading space thing

[Patrick Ohly]

On Mon, 2017-02-27 at 06:07 -0500, Robert P. J. Day wrote:
>   getting pedantic (as i am wont to do), occasionally i run across
> things like this, from meta/conf/distro/include/no-static-libs.inc:
> 
>   EXTRA_OECONF_append = "${DISABLE_STATIC}"
> 
> which, at first glance, simply seems wrong given the lack of a leading
> space, until one looks higher up in that same file to read:
> 
>   DISABLE_STATIC = " --disable-static"
> 
> where one finds the leading space as part of the variable itself. i
> find this potentially *really* confusing; consistent style suggests
> variables should be assigned their values without regard as to how
> they might be used later. subsequent references or usages of those
> variables should be responsible for making sure they're included
> properly, no?

In principle you are right, but probably it was done this way here to
avoid adding a space to EXTRA_OECONF in those cases where DISABLE_STATIC
is overridden to be empty. Consider DISABLE_STATIC an implementation
detail and then it becomes okay to set it as needed by the
implementation.

>   and the last line in that same file also suggests potential misuse:
> 
>   EXTRA_OECMAKE_append_pn-libical = "-DSHARED_ONLY=True"

Yes, that's just plain wrong and just happens to work by accident.

> p.s. would the same logic hold with lines like this?
> 
> meta/conf/machine/include/tune-ppce6500.inc:
> 
>   MACHINE_FEATURES_BACKFILL_CONSIDERED_append =
>      "${@bb.utils.contains('TUNE_FEATURES', 'e6500', ' qemu-usermode', '', d)}"
> 
> would it not be clearer if one were to write:
> 
>   MACHINE_FEATURES_BACKFILL_CONSIDERED_append =
>      " ${@bb.utils.contains('TUNE_FEATURES', 'e6500', 'qemu-usermode', '', d)}"

The existing line is better because it avoids adding a space when
nothing needs to be added.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: style suggestions for that whole "_append" and leading space thing

[Robert P. J. Day]

On Tue, 28 Feb 2017, Patrick Ohly wrote:

> On Mon, 2017-02-27 at 06:07 -0500, Robert P. J. Day wrote:
> >   getting pedantic (as i am wont to do), occasionally i run across
> > things like this, from meta/conf/distro/include/no-static-libs.inc:
> >
> >   EXTRA_OECONF_append = "${DISABLE_STATIC}"
> >
> > which, at first glance, simply seems wrong given the lack of a leading
> > space, until one looks higher up in that same file to read:
> >
> >   DISABLE_STATIC = " --disable-static"
> >
> > where one finds the leading space as part of the variable itself.
> > i find this potentially *really* confusing; consistent style
> > suggests variables should be assigned their values without regard
> > as to how they might be used later. subsequent references or
> > usages of those variables should be responsible for making sure
> > they're included properly, no?
>
> In principle you are right, but probably it was done this way here
> to avoid adding a space to EXTRA_OECONF in those cases where
> DISABLE_STATIC is overridden to be empty. Consider DISABLE_STATIC an
> implementation detail and then it becomes okay to set it as needed
> by the implementation.

  ehhhhhhhhh ... i'm not sure i'm convinced but i'll let this one go.

> >   and the last line in that same file also suggests potential misuse:
> >
> >   EXTRA_OECMAKE_append_pn-libical = "-DSHARED_ONLY=True"
>
> Yes, that's just plain wrong and just happens to work by accident.

  just submitted a patch for this.

> > p.s. would the same logic hold with lines like this?
> >
> > meta/conf/machine/include/tune-ppce6500.inc:
> >
> >   MACHINE_FEATURES_BACKFILL_CONSIDERED_append =
> >      "${@bb.utils.contains('TUNE_FEATURES', 'e6500', ' qemu-usermode', '', d)}"
> >
> > would it not be clearer if one were to write:
> >
> >   MACHINE_FEATURES_BACKFILL_CONSIDERED_append =
> >      " ${@bb.utils.contains('TUNE_FEATURES', 'e6500', 'qemu-usermode', '', d)}"
>
> The existing line is better because it avoids adding a space when
> nothing needs to be added.

  ok, i'll buy that.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================