Re: [XEN PATCH v2 1/4] build,include: rework shell script for headers++.chk

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Jun 15, 2022 at 08:37:42AM +0200, Jan Beulich wrote:
> On 14.06.2022 18:22, Anthony PERARD wrote:
> > diff --git a/xen/include/Makefile b/xen/include/Makefile
> > index 6d9bcc19b0..49c75a78f9 100644
> > --- a/xen/include/Makefile
> > +++ b/xen/include/Makefile
> > @@ -158,13 +158,22 @@ define cmd_headerscxx_chk
> >         touch $@.new;                                                     \
> >         exit 0;                                                           \
> >     fi;                                                                   \
> > -   $(foreach i, $(filter %.h,$^),                                        \
> > -       echo "#include "\"$(i)\"                                          \
> > +   get_prereq() {                                                        \
> > +       case $$1 in                                                       \
> > +       $(foreach i, $(filter %.h,$^),                                    \
> > +       $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
> > +           $(i)$(close)                                                  \
> > +           echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
> > +                   -include c$(j))";;))                                  \
> > +       esac;                                                             \
> 
> If I'm reading this right (indentation looks to be a little misleading
> and hence one needs to count parentheses) the case statement could (in
> theory) remain without any "body". As per the command language spec I'm
> looking at this (if it works in the first place) is an extension, and
> formally there's always at least one label required. Since we aim to be
> portable in such regards, I'd like to request that there be a final
> otherwise empty *) line.

When looking at the shell grammar at [1], an empty body seems to be
allowed. But I can add "*)" at the end for peace of mind.

[1] 
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_10_02

As for misleading indentation, I've got my $EDITOR to show me matching
parentheses, so I don't need to count them. But if I rewrite that
function as following, would it be easier to follow?

+       get_prereq() {                                                        \
+       case $$1 in                                                           \
+       $(foreach i, $(filter %.h,$^),                                        \
+           $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
+               $(i)$(close)                                                  \
+               echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
+                       -include c$(j))";;))                                  \
+       *) ;;                                                                 \
+       esac;                                                                 \
+       };                                                                    \


> > +   };                                                                    \
> > +   for i in $(filter %.h,$^); do                                         \
> > +       echo "#include "\"$$i\"                                           \
> >         | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__        \
> >           -include stdint.h -include $(srcdir)/public/xen.h               \
> > -         $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include 
> > c$(j)) \
> > +         `get_prereq $$i`                                                \
> 
> While I know we use back-ticked quoting elsewhere, I'd generally
> recommend to use $() for readability. But maybe others view this
> exactly the other way around ...

Well, in Makefile it's `` vs $$(). At least, we don't have to write
$$$(open)$(close) here :-).

I guess $$(get_prereq $$i) isn't too bad here, I'll replace the
backquote.

> And a question without good context to put at: Isn't headers99.chk in
> similar need of converting? It looks only slightly less involved than
> the C++ one.

It doesn't really need converting at the moment, because there's only
two headers to check, so the resulting command line is small. But
converting it at the same time is probably a good idea to avoid having
two different implementations of the header check.

Thanks,

-- 
Anthony PERARD