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
