Re: [XEN PATCH v7 01/51] build: introduce cpp_flags macro

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Sep 02, 2021 at 12:08:58PM +0200, Jan Beulich wrote:
> On 24.08.2021 12:49, Anthony PERARD wrote:
> > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> albeit with a remark:
> 
> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -133,6 +133,9 @@ endif
> >  # Always build obj-bin files as binary even if they come from C source. 
> >  $(obj-bin-y): XEN_CFLAGS := $(filter-out -flto,$(XEN_CFLAGS))
> >  
> > +# To be use with $(a_flags) or $(c_flags) to produce CPP flags
> > +cpp_flags = $(filter-out -Wa$(comma)% -flto,$(1))
> 
> Afaics this has nothing to do with Linux'es cpp_flags, so what we do here
> is entirely up to us. If this is strictly intended to be used the another
> macro, wouldn't it make sense to have
> 
> cpp_flags = $(filter-out -Wa$(comma)% -flto,$($(1)))
> 
> here and then e.g. ...
> 
> > @@ -222,13 +225,13 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): 
> > %.init.o: %.o FORCE
> >     $(call if_changed,obj_init_o)
> >  
> >  quiet_cmd_cpp_i_c = CPP     $@
> > -cmd_cpp_i_c = $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -MQ $@ -o $@ $<
> > +cmd_cpp_i_c = $(CPP) $(call cpp_flags,$(c_flags)) -MQ $@ -o $@ $<
> 
> ... the slightly simpler / easier to read
> 
> cmd_cpp_i_c = $(CPP) $(call cpp_flags,c_flags) -MQ $@ -o $@ $<
> 
> here?

I don't think this is better or simpler. "cpp_flags" don't need to know
the name of the variable to be useful. I think it is better to know that
"cpp_flags" act on the value of the variable rather than the variable
itself, when reading "$(call cpp_flags, $(a_flags))".

But thanks for the suggestion, and for the review,

-- 
Anthony PERARD