Re: [Xen-devel] [PATCH v7 2/4] xen: introduce a C99 headers check

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 31 Mar 2017, Jan Beulich wrote:
> >>> On 30.03.17 at 23:04, <sstabellini@xxxxxxxxxx> wrote:
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -274,6 +274,7 @@ xen/arch/*/efi/compat.c
> >  xen/arch/*/efi/efi.h
> >  xen/arch/*/efi/runtime.c
> >  xen/include/headers.chk
> > +xen/include/headers99.chk
> >  xen/include/headers++.chk
> 
> I'm sorry for not having noticed on the previous round, but please
> make this xen/include/headers*.chk just like was done in the clean
> target. Generally clean targets and .gitignore entries should match.

Sure


> > @@ -104,16 +105,21 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
> >     done >$@.new
> >     mv $@.new $@
> >  
> > +headers99.chk: $(PUBLIC_C99_HEADERS) Makefile
> > +   rm -f $@.new
> > +   $(foreach i, $(filter %.h,$^), $(CC) -x c -std=c99 -Wall -Werror    \
> > +       -include stdint.h $(foreach j, $($(i)-prereq), -include $(j).h) \
> > +       -S -o /dev/null $(i) || exit 1; echo $(i) >> $@.new;)
> > +   mv $@.new $@
> > +
> >  headers++.chk: $(PUBLIC_HEADERS) Makefile
> > -   if $(CXX) -v >/dev/null 2>&1; then \
> > -       for i in $(filter %.h,$^); do \
> > -           echo '#include "'$$i'"' \
> > -           | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
> > -             -include stdint.h -include public/xen.h -S -o /dev/null - \
> > -           || exit 1; \
> > -           echo $$i; \
> > -       done ; \
> > -   fi >$@.new
> > +   rm -f $@.new
> > +   $(CXX) -v >/dev/null 2>&1 || exit 0
> 
> As said before, the lack of a semicolon and line continuation here
> will result in this not dong what you want: If there's no C++
> compiler available, you'll exit only the shell which was invoked to
> execute above command, signaling success to make, which will
> then continue with the next command in the rule, which will then
> fail due to there not being a C++ compiler.

I think understand now, I'll fix.


> > +   $(foreach i, $(filter %.h,$^), echo "#include "\"$(i)\"|       \
> 
> Preferably I'd like to see the | on the start of the next line, as it
> was before (the same would then go for the later ||). Alternatively
> at least have a blank between closing quote and pipeline character.

I moved | and || at the beginning of the next lines.


> > +       $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__   \
> > +       -include stdint.h -include public/xen.h                    \
> > +       $(foreach j, $($(i)-prereq), -include c$(j))               \
> > +       -S -o /dev/null - || exit 1; echo $(i) >> $@.new;)
> 
> Would you mind switching to "exit $$?" instead of "exit 1" here,
> to propagate the exit code from the compiler? Also in the C99
> case then. An alternative would be to use "set -e" up front.

I can do that


> I'm sorry, I again should have noticed these last two earlier.

That's OK. Thanks for the quick reviews.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel