Re: [XEN PATCH v3 05/16] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-06-26 11:06, Jan Beulich wrote:
> On 25.06.2024 21:31, Nicola Vetrini wrote:
> > On 2024-03-12 09:16, Jan Beulich wrote:
> > > On 11.03.2024 09:59, Simone Ballarin wrote:
> > > > --- a/xen/arch/x86/Makefile
> > > > +++ b/xen/arch/x86/Makefile
> > > > @@ -258,18 +258,20 @@ $(obj)/asm-macros.i: CFLAGS-y += -P
> > > >  $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i
> > > > $(src)/Makefile
> > > >         $(call filechk,asm-macros.h)
> > > >
> > > > +ARCHDIR = $(shell echo $(SRCARCH) | tr a-z A-Z)
> > > This wants to use :=, I think - there's no reason to invoke the shell
> > > ...
> > I agree on this
> >
> > > >  define filechk_asm-macros.h
> > > > +    echo '#ifndef ASM_$(ARCHDIR)_ASM_MACROS_H'; \
> > > > +    echo '#define ASM_$(ARCHDIR)_ASM_MACROS_H'; \
> > > >      echo '#if 0'; \
> > > >      echo '.if 0'; \
> > > >      echo '#endif'; \
> > > > -    echo '#ifndef __ASM_MACROS_H__'; \
> > > > -    echo '#define __ASM_MACROS_H__'; \
> > > >      echo 'asm ( ".include \"$@\"" );'; \
> > > > -    echo '#endif /* __ASM_MACROS_H__ */'; \
> > > >      echo '#if 0'; \
> > > >      echo '.endif'; \
> > > >      cat $<; \
> > > > -    echo '#endif'
> > > > +    echo '#endif'; \
> > > > +    echo '#endif /* ASM_$(ARCHDIR)_ASM_MACROS_H */'
> > > >  endef
> > > ... three times while expanding this macro. Alternatively (to avoid
> > > an unnecessary shell invocation when this macro is never expanded at
> > > all) a shell variable inside the "define" above would want 
> > > introducing.
> > > Whether this 2nd approach is better depends on whether we anticipate
> > > further uses of ARCHDIR.
> > However here I'm not entirely sure about the meaning of this latter
> > proposal.
> > My proposal is the following:
> >
> > ARCHDIR := $(shell echo $(SRCARCH) | tr a-z A-Z)
> >
> > in a suitably generic place (such as Kbuild.include or maybe
> > xen/Makefile) as you suggested in subsequent patches that reused this
> > pattern.
> If $(ARCHDIR) is going to be used elsewhere, then what you suggest is 
> fine.
> My "whether" in the earlier reply specifically left open for 
> clarification
> what the intentions with the variable are. The alternative I had 
> described
> makes sense only when $(ARCHDIR) would only ever be used inside the
> filechk_asm-macros.h macro.
Yes, the intention is to reuse $(ARCHDIR) in the formation of other 
places, as you can tell from the fact that subsequent patches replicate 
the same pattern. This is going to save some duplication.
The only matter left then is whether xen/Makefile (around line 250, just 
after setting SRCARCH) would be better, or Kbuild.include. To me the 
former place seems more natural, but I'm not totally sure.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)