Re: [XEN PATCH v4 10/18] xen/build: use if_changed on built_in.o

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Apr 08, 2020 at 03:35:17PM +0200, Jan Beulich wrote:
> On 08.04.2020 15:13, Andrew Cooper wrote:
> > On 08/04/2020 13:40, Jan Beulich wrote:
> >> On 31.03.2020 12:30, Anthony PERARD wrote:
> >>> --- a/xen/Rules.mk
> >>> +++ b/xen/Rules.mk
> >>> @@ -130,15 +130,24 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk
> >>>  c_flags += $(CFLAGS-y)
> >>>  a_flags += $(CFLAGS-y) $(AFLAGS-y)
> >>>  
> >>> -built_in.o: $(obj-y) $(extra-y)
> >>> -ifeq ($(obj-y),)
> >>> - $(CC) $(c_flags) -c -x c /dev/null -o $@
> >>> -else
> >>> +quiet_cmd_ld_builtin = LD      $@
> >>>  ifeq ($(CONFIG_LTO),y)
> >>> - $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^)
> >>> +cmd_ld_builtin = \
> >>> +    $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs))
> >>>  else
> >>> - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^)
> >>> +cmd_ld_builtin = \
> >>> +    $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out 
> >>> $(extra-y),$(real-prereqs))
> >>>  endif
> >> How about going yet one step further and doing away with the
> >> ifeq here altogether:
> >>
> >> cmd_ld_builtin-y = \
> >>     $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs))
> >> cmd_ld_builtin-$(CONFIG_LTO) = \
> >>     $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs))
> > 
> > Please don't.
> > 
> > Logic like this is substantially harder to follow than a plain if/else
> > construct, and clarity is of far higher importance than optimising the
> > line count in the build system.
> 
> I could maybe see the argument if the two definitions were far apart.
> This suggestion isn't about line count at all, but about clarity. In
> particular because of the need to use ifeq(,) rather than simple "if"
> constructs, I view this list model as the better alternative in all
> cases where it can be made use of.

We could use "ifdef CONFIG_LTO" to avoid ifeq ;-). But I think you
disliked that because CONFIG_LTO could be present in the environment
with a value different than "y" and mess up the build system, just to
annoy us.

> > This trick only works for trivial cases, and interferes with diff's when
> > the logic inevitably becomes less trivial.
> 
> It may, but whether it actually will can't be known until such time
> as it would get touched. The suggested model may very well also be
> suitable then.
> 
> Anyway, Anthony, I'm not going to insist. This is just another aspect
> where we would better generally settle on the preferred style to use.

I think if/else is better for alternatives. And we can keep "var-y" for
lists with optional items.

Thanks,

-- 
Anthony PERARD