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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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.

> +     $(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.

> +         $(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'm sorry, I again should have noticed these last two earlier.

Jan


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