member inconsistency?

member inconsistency?

[Nicolas Goaziou]

Hello,

The following behaviour is surprising

  (assq 'a 'b)   => #ERROR
  (memq 'a 'b)   => #ERROR
  (assoc 'a 'b)  => #ERROR

so far so good, but

  (member 'a 'b) => nil

I was caught by this when replacing (member SYMBOL ...) with (memq
SYMBOL ...). Is it intentional? Should I file a bug report instead?


Regards,

-- 
Nicolas Goaziou                                                0x80A93738

Re: member inconsistency?

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

Nicolas Goaziou <mail@nicolasgoaziou.fr> schrieb am Do., 28. Jan. 2016 um
09:41 Uhr:

> Hello,
>
> The following behaviour is surprising
>
>   (assq 'a 'b)   => #ERROR
>   (memq 'a 'b)   => #ERROR
>   (assoc 'a 'b)  => #ERROR
>

For completeness, memql and assq have the same behavior.


>
> so far so good, but
>
>   (member 'a 'b) => nil
>
> I was caught by this when replacing (member SYMBOL ...) with (memq
> SYMBOL ...). Is it intentional? Should I file a bug report instead?
>

Yes, I think this is a bug. There is no reason why `member' should be so
inconsistent, especially given that is has (almost) the same docstring as
memq and memql.

[-- Attachment #2: Type: text/html, Size: 1172 bytes --]

Re: member inconsistency?

[Andreas Schwab]

Philipp Stephani <p.stephani2@gmail.com> writes:

> Nicolas Goaziou <mail@nicolasgoaziou.fr> schrieb am Do., 28. Jan. 2016 um
> 09:41 Uhr:
>
>> Hello,
>>
>> The following behaviour is surprising
>>
>>   (assq 'a 'b)   => #ERROR
>>   (memq 'a 'b)   => #ERROR
>>   (assoc 'a 'b)  => #ERROR
>>
>
> For completeness, memql and assq have the same behavior.

memql works like member if looking for a float, like memq otherwise.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: member inconsistency?

[Stephen Berman]

On Thu, 28 Jan 2016 11:14:37 +0100 Andreas Schwab <schwab@suse.de> wrote:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
>> Nicolas Goaziou <mail@nicolasgoaziou.fr> schrieb am Do., 28. Jan. 2016 um
>> 09:41 Uhr:
>>
>>> Hello,
>>>
>>> The following behaviour is surprising
>>>
>>>   (assq 'a 'b)   => #ERROR
>>>   (memq 'a 'b)   => #ERROR
>>>   (assoc 'a 'b)  => #ERROR
>>>
>>
>> For completeness, memql and assq have the same behavior.
>
> memql works like member if looking for a float, like memq otherwise.

The definition of `memq' calls CHECK_LIST (list) and `assq' and `assoc'
return CAR (list), and each of these signals the error in case of a
wrong type.  In contrast, `member' (and `memql' in the float case) has
no check and also uses a for-loop with the condition CONSP (tail), and
when this fails, the function just returns Qnil.  Adding CHECK_LIST
(list) before the for-loop makes (member 'a 'b) signal an error.
(Instead of a for-loop, the other three use while (1), so the loop
always runs and the type checks get executed.)

Steve Berman

Re: member inconsistency?

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Date: Thu, 28 Jan 2016 11:25:33 +0100
> Cc: Philipp Stephani <p.stephani2@gmail.com>,
> 	Nicolas Goaziou <mail@nicolasgoaziou.fr>, emacs-devel@gnu.org
> 
> In contrast, `member' (and `memql' in the float case) has no check
> and also uses a for-loop with the condition CONSP (tail), and when
> this fails, the function just returns Qnil.  Adding CHECK_LIST
> (list) before the for-loop makes (member 'a 'b) signal an error.

I'd say CHECK_LIST_CONS, not CHECK_LIST, don't you agree?

Re: member inconsistency?

[Stephen Berman]

On Thu, 28 Jan 2016 17:48:29 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Date: Thu, 28 Jan 2016 11:25:33 +0100
>> Cc: Philipp Stephani <p.stephani2@gmail.com>,
>> 	Nicolas Goaziou <mail@nicolasgoaziou.fr>, emacs-devel@gnu.org
>> 
>> In contrast, `member' (and `memql' in the float case) has no check
>> and also uses a for-loop with the condition CONSP (tail), and when
>> this fails, the function just returns Qnil.  Adding CHECK_LIST
>> (list) before the for-loop makes (member 'a 'b) signal an error.
>
> I'd say CHECK_LIST_CONS, not CHECK_LIST, don't you agree?

To be honest, I overlooked that CHECK_LIST_CONS is inside the for-loop.
But it seems a bit ugly to have the same check before the loop and
inside it as well.  How about the following:

diff --git a/src/fns.c b/src/fns.c
index 86ad333..17c4a75 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -1349,7 +1349,7 @@ The value is actually the tail of LIST whose car is ELT.  */)
   (register Lisp_Object elt, Lisp_Object list)
 {
   register Lisp_Object tail;
-  for (tail = list; CONSP (tail); tail = XCDR (tail))
+  for (tail = list; CONSP (tail) || !NILP (tail); tail = XCDR (tail))
     {
       register Lisp_Object tem;
       CHECK_LIST_CONS (tail, list);

Or is having the check before the loop better it catches the error sooner?

Steve Berman

Re: member inconsistency?

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: schwab@suse.de,  p.stephani2@gmail.com,  mail@nicolasgoaziou.fr,  emacs-devel@gnu.org
> Date: Thu, 28 Jan 2016 18:30:04 +0100
> 
> diff --git a/src/fns.c b/src/fns.c
> index 86ad333..17c4a75 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -1349,7 +1349,7 @@ The value is actually the tail of LIST whose car is ELT.  */)
>    (register Lisp_Object elt, Lisp_Object list)
>  {
>    register Lisp_Object tail;
> -  for (tail = list; CONSP (tail); tail = XCDR (tail))
> +  for (tail = list; CONSP (tail) || !NILP (tail); tail = XCDR (tail))
>      {
>        register Lisp_Object tem;
>        CHECK_LIST_CONS (tail, list);
> 
> Or is having the check before the loop better it catches the error sooner?

Before the loop sounds slightly less obscure to me.

Thanks.

Re: member inconsistency?

[Stephen Berman]

On Thu, 28 Jan 2016 20:35:40 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: schwab@suse.de,  p.stephani2@gmail.com,  mail@nicolasgoaziou.fr,  emacs-devel@gnu.org
>> Date: Thu, 28 Jan 2016 18:30:04 +0100
>> 
>> diff --git a/src/fns.c b/src/fns.c
>> index 86ad333..17c4a75 100644
>> --- a/src/fns.c
>> +++ b/src/fns.c
>> @@ -1349,7 +1349,7 @@ The value is actually the tail of LIST whose car is ELT.  */)
>>    (register Lisp_Object elt, Lisp_Object list)
>>  {
>>    register Lisp_Object tail;
>> -  for (tail = list; CONSP (tail); tail = XCDR (tail))
>> +  for (tail = list; CONSP (tail) || !NILP (tail); tail = XCDR (tail))
>>      {
>>        register Lisp_Object tem;
>>        CHECK_LIST_CONS (tail, list);
>> 
>> Or is having the check before the loop better it catches the error sooner?
>
> Before the loop sounds slightly less obscure to me.

With just
   CHECK_LIST_CONS (list, list);
before the loop, the build fails with "wrong-type-argument: nil, listp"
in subr.el.  With 
   if (!NILP (list))
     CHECK_LIST_CONS (list, list);
before the loop, the build succeeds (same with Fmemql).  But isn't this
then effectively the same thing as having CHECK_LIST (list) before the
loop?  With that, the build also succeeds and I didn't see any problems
with calls to member.  I'm confused.

Two other things I don't get: (1) I wondered why Fmember can't be
defined with a while (1) loop analogously to Fmemq, Fassq und Fassoc,
but when I tried that the build failed at
   temacs --batch --load loadup bootstrap
even though I added a !NILP (list) check.
(2) Why are there three occurrences in a row of these lines in Fmemq
(and similar repetitions in Fassq and Fassoc)?
   if (!CONSP (list) || EQ (XCAR (list), elt))
     break;
   list = XCDR (list);
I commented out two of the occurrences and built emacs successfully and
all my tests of memq went as expected.  So why the repetitions?

Steve Berman

Re: member inconsistency?

[Andreas Schwab]

Stephen Berman <stephen.berman@gmx.net> writes:

> diff --git a/src/fns.c b/src/fns.c
> index 86ad333..17c4a75 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -1349,7 +1349,7 @@ The value is actually the tail of LIST whose car is ELT.  */)
>    (register Lisp_Object elt, Lisp_Object list)
>  {
>    register Lisp_Object tail;
> -  for (tail = list; CONSP (tail); tail = XCDR (tail))
> +  for (tail = list; CONSP (tail) || !NILP (tail); tail = XCDR (tail))
>      {
>        register Lisp_Object tem;
>        CHECK_LIST_CONS (tail, list);

The check for CONSP in the loop condition is redundant, that is already
checked by CHECK_LIST_CONS (and the latter was a no-op before).

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: member inconsistency?

[Stephen Berman]

On Thu, 28 Jan 2016 23:18:29 +0100 Andreas Schwab <schwab@suse.de> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> diff --git a/src/fns.c b/src/fns.c
>> index 86ad333..17c4a75 100644
>> --- a/src/fns.c
>> +++ b/src/fns.c
>> @@ -1349,7 +1349,7 @@ The value is actually the tail of LIST whose car is ELT.  */)
>>    (register Lisp_Object elt, Lisp_Object list)
>>  {
>>    register Lisp_Object tail;
>> -  for (tail = list; CONSP (tail); tail = XCDR (tail))
>> +  for (tail = list; CONSP (tail) || !NILP (tail); tail = XCDR (tail))
>>      {
>>        register Lisp_Object tem;
>>        CHECK_LIST_CONS (tail, list);
>
> The check for CONSP in the loop condition is redundant, that is already
> checked by CHECK_LIST_CONS (and the latter was a no-op before).

Thanks.  So is the following patch ok to commit to emacs-25?

diff --git a/src/fns.c b/src/fns.c
index 86ad333..b75f0c8 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -1349,7 +1349,7 @@ The value is actually the tail of LIST whose car is ELT.  */)
   (register Lisp_Object elt, Lisp_Object list)
 {
   register Lisp_Object tail;
-  for (tail = list; CONSP (tail); tail = XCDR (tail))
+  for (tail = list; !NILP (tail); tail = XCDR (tail))
     {
       register Lisp_Object tem;
       CHECK_LIST_CONS (tail, list);
@@ -1397,7 +1397,7 @@ The value is actually the tail of LIST whose car is ELT.  */)
   if (!FLOATP (elt))
     return Fmemq (elt, list);
 
-  for (tail = list; CONSP (tail); tail = XCDR (tail))
+  for (tail = list; !NILP (tail); tail = XCDR (tail))
     {
       register Lisp_Object tem;
       CHECK_LIST_CONS (tail, list);

Re: member inconsistency?

[Johan Bockgård]

Stephen Berman <stephen.berman@gmx.net> writes:

> In contrast, `member' (and `memql' in the float case) has no check and
> also uses a for-loop with the condition CONSP (tail), and when this
> fails, the function just returns Qnil.

`delete' and `delq' also has the bug.

http://debbugs.gnu.org/264

Re: member inconsistency?

[Stephen Berman]

On Fri, 29 Jan 2016 00:38:24 +0100 Johan Bockgård <bojohan@gnu.org> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> In contrast, `member' (and `memql' in the float case) has no check and
>> also uses a for-loop with the condition CONSP (tail), and when this
>> fails, the function just returns Qnil.
>
> `delete' and `delq' also has the bug.
>
> http://debbugs.gnu.org/264

Oh, thanks for the pointer.  So the patch I proposed reverts a change
Stefan deliberately made more than eight years ago.  That's not
something I feel entitled to do.

Steve Berman

Re: member inconsistency?

[Andreas Schwab]

Stephen Berman <stephen.berman@gmx.net> writes:

> Oh, thanks for the pointer.  So the patch I proposed reverts a change
> Stefan deliberately made more than eight years ago.

IMHO that change was wrong.  It made CHECK_LIST_CONS a no-op, and there
was nothing wrong with it before Stefan's change.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: member inconsistency?

[Stephen Berman]

On Fri, 29 Jan 2016 11:25:42 +0100 Andreas Schwab <schwab@suse.de> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> Oh, thanks for the pointer.  So the patch I proposed reverts a change
>> Stefan deliberately made more than eight years ago.
>
> IMHO that change was wrong.  It made CHECK_LIST_CONS a no-op, and there
> was nothing wrong with it before Stefan's change.

AFAIU the code, I agree with you, but Stefan evidently didn't see it
that way at the time, and while the ChangeLog doesn't indicate whether a
problem was being fixed (and I found no discussion about it by skimming
through the emacs-devel archive), he disputed there was a bug when Johan
Bockgård reported exactly this issue a few months after the change.  But
I don't feel competent enough to argue against it, so I'm bowing out here.

Steve Berman

Re: member inconsistency?

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Date: Fri, 29 Jan 2016 11:25:42 +0100
> Cc: Philipp Stephani <p.stephani2@gmail.com>,
> 	Nicolas Goaziou <mail@nicolasgoaziou.fr>,
> 	Johan Bockgård <bojohan@gnu.org>, emacs-devel@gnu.org
> 
> Stephen Berman <stephen.berman@gmx.net> writes:
> 
> > Oh, thanks for the pointer.  So the patch I proposed reverts a change
> > Stefan deliberately made more than eight years ago.
> 
> IMHO that change was wrong.  It made CHECK_LIST_CONS a no-op, and there
> was nothing wrong with it before Stefan's change.

But given that we've lived with it for more than 8 years, the revert
should have been done on master, not the release branch, IMO.

Re: member inconsistency?

[John Wiegley]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

>> > Oh, thanks for the pointer.  So the patch I proposed reverts a change
>> > Stefan deliberately made more than eight years ago.
>> 
>> IMHO that change was wrong.  It made CHECK_LIST_CONS a no-op, and there
>> was nothing wrong with it before Stefan's change.

> But given that we've lived with it for more than 8 years, the revert should
> have been done on master, not the release branch, IMO.

Agreed.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: member inconsistency?

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > In contrast, `member' (and `memql' in the float case) has no check and
  > > also uses a for-loop with the condition CONSP (tail), and when this
  > > fails, the function just returns Qnil.

  > `delete' and `delq' also has the bug.

Why consider it a bug?  What is wrong with it?

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: member inconsistency?

[Fabrice Popineau]

Richard Stallman <rms <at> gnu.org> writes:

> 
> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
> 
>   > > In contrast, `member' (and `memql' in the float case) has no check 
and
>   > > also uses a for-loop with the condition CONSP (tail), and when 
this
>   > > fails, the function just returns Qnil.
> 
>   > `delete' and `delq' also has the bug.
> 
> Why consider it a bug?  What is wrong with it?
> 

It makes it harder to catch the bug if you
inadvertently call member with its second
argument not being a sequence ?

Fabrice

Re: member inconsistency?

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > It makes it harder to catch the bug if you
  > inadvertently call member with its second
  > argument not being a sequence ?

There are two ways to look at this:

* Lists should end in nil, and if they don't, functions should complain.

* It is useful generality if functions accept lists that end
in some other atom.

I don't see that the first is obviously preferable.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: member inconsistency?

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

2016-02-01 12:04 GMT+01:00 Richard Stallman <rms@gnu.org>:

> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>
>   > It makes it harder to catch the bug if you
>   > inadvertently call member with its second
>   > argument not being a sequence ?
>
> There are two ways to look at this:
>
> * Lists should end in nil, and if they don't, functions should complain.
>
> * It is useful generality if functions accept lists that end
> in some other atom.
>
> I don't see that the first is obviously preferable.
>
>
I'm biased by CL I guess.
Then you need 2 functions, one for proper lists and one for improper lists.
But I would call the second one `tree-member' :-)

Greetings,

Fabrice

[-- Attachment #2: Type: text/html, Size: 1368 bytes --]