Re: [Qemu-devel] [PATCH 01/47] rules.mak: New logical functions

[Peter Maydell]

On 25 August 2013 23:58, Ákos Kovács <address@hidden> wrote:
> lnot, land, lor, lif, eq, ne, isempty, notempty functions added
> Example usage:
>     obj-$(call lor,$(CONFIG_LINUX),$(CONFIG_BSD)) += feature.o
>
> Signed-off-by: Ákos Kovács <address@hidden>

Hi; I like the general idea here but I think there
are some issues with your implementation.

> ---
>  rules.mak |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/rules.mak b/rules.mak
> index 4499745..7e8e3bd 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -106,6 +106,22 @@ clean: clean-timestamp
>  obj := .
>  old-nested-dirs :=
>
> +# Logical functions

I think we should be clear here about the input
and output ranges of these functions, ie that
the inputs should always be "y", "n" or "" (with
either "n" or the empty string being false).

> +lnot = $(if $(subst n,,$1),n,y)
> +
> +land-yy = y
> +land = $(land-$1$2)

This means that $(call land,y,n) would expand
to the empty string, not 'y' or 'n', right?
I think we should make all these functions always
expand to either 'y' or 'n'.

land = $(if $(findstring yy,$1$2),y,n)

will do this.

> +lor = $(if $(subst $2,,$1)$(subst $1,,$2),n,y)

This can't be correct for both 'lor' and 'eq'.
In fact it works as 'eq'. A working lor would be:

lor = $(if $(findstring y,$1$2),y,n)

> +lif = $(if $(subst n,,$1),$2,$3)
> +
> +eq = $(if $(subst $2,,$1)$(subst $1,,$2),n,y)
> +ne = $(if $(subst $2,,$1)$(subst $1,,$2),y,n)

These give the wrong answer for comparisons
of 'n' with ''. Working versions:

eq = $(if $(filter $(call lnot,$1),$(call lnot,$2)),y,n)
ne = $(if $(filter $(call lnot,$1),$(call lnot,$2)),n,y)

> +isempty = $(call eq,$1,)
> +notempty = $(call ne,$1,)

These are wrong assuming we want 'n' eq '' semantics,
and we can directly implement them with $if anyway:

isempty = $(if $1,n,y)
notempty = $(if $1,y,n)

Below is the test makefile I used to check these:
===begin===
# insert function definitions here

yes=y
no=n
full=full
empty=
# change this to whichever fn you are looking at
testfn=ne


.PHONY: all

all:
    @echo "$(testfn) yes,yes = $(call $(testfn),$(yes),$(yes))"
    @echo "$(testfn) yes,no = $(call $(testfn),$(yes),$(no))"
    @echo "$(testfn) no,yes = $(call $(testfn),$(no),$(yes))"
    @echo "$(testfn) no,no = $(call $(testfn),$(no),$(no))"
    @echo "$(testfn) yes,empty = $(call $(testfn),$(yes),$(empty))"
    @echo "$(testfn) empty,yes = $(call $(testfn),$(empty),$(yes))"
    @echo "$(testfn) empty,no = $(call $(testfn),$(empty),$(no))"
    @echo "$(testfn) no,empty = $(call $(testfn),$(no),$(empty))"
    @echo "$(testfn) empty,empty = $(call $(testfn),$(empty),$(empty))"
    @echo "isempty($(empty)) = $(call isempty,$(empty))"
    @echo "isempty($(full)) = $(call isempty,$(full))"
    @echo "notempty($(empty)) = $(call notempty,$(empty))"
    @echo "notempty($(full)) = $(call notempty,$(full))"
===endit===

Since I've effectively rewritten all your patch here :-)
I'll put together a patch plus your patches 2 and 3
from this series since I think they make a nice standalone
cleanup.

Thanks
-- PMM