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

[Paolo Bonzini]

Il 13/09/2013 15:43, Peter Maydell ha scritto:
> 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/notempty are clearly string functions, where only the output is
of the y/n form.  Seeing Akos's implementation of isempty/notempty, I
think the desired semantics for eq/ne/isempty/notempty are also those of
string functions.

I would call your functions leqv/lxor, not eq/ne.

Your patch is fine if you either rename eq/ne like this, or revert them
to Akos's version.

Paolo

>> +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)