bug#41354: equal? has no sensible code path for symbols

Hi,

David Kastrup <[email protected]> skribis:

> Ludovic Courtès <[email protected]> writes:
>
>> Hi David,
>>
>> David Kastrup <[email protected]> skribis:
>>
>>> In Scheme, symbols can be compared using eq? for equality.  However,
>>> since they have garbage-collected content attached, they do not meet the
>>> predicate SCM_IMP in the short-circuit evaluation at the start of equal?
>>> This means that unequal symbols compared using equal? fall through a
>>> whole bunch of tests and end up in a general structural comparison
>>> comparing their underlying string names.
>>
>> ‘equal?’ starts by checking for eq-ness, which LGTM:
>>
>>   SCM
>>   scm_equal_p (SCM x, SCM y)
>>   #define FUNC_NAME s_scm_i_equal_p
>>   {
>>     SCM_CHECK_STACK;
>>    tailrecurse:
>>     SCM_TICK;
>>     if (scm_is_eq (x, y))
>>       return SCM_BOOL_T;
>>
>> Or were you referring to something else?
>
> I repeat: "This means that UNEQUAL symbols compared using equal? fall
> through a whole bunch of tests and end up in a general structural
> comparison comparing their underlying string names".
>
> Lots of searches _end_ with an equal comparison (which is fast) but do a
> lot of unequal comparisons before that (which is slow, even though
> symbols that are not eq? will also not be equal?, so if you know you are
> checking _symbols_, if they are not eq? you are done).
>
> Symbols comparing as _unequal_ have no special path in equal?.

I was going to say that this is necessary for uninterned symbols, but it
turns out that uninterned symbols that look the same are not ‘equal?’:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> (define a (make-symbol "x"))
scheme@(guile-user)> (define b (make-symbol "x"))
scheme@(guile-user)> (eq? a b)
$10 = #f
scheme@(guile-user)> (equal? a b)
$11 = #f
--8<---------------cut here---------------end--------------->8---

Thus we could go with the patch below, though I doubt it would make a
measurable difference (and it actually adds tests for other cases).

Thoughts?

Besides, in the common case where one is comparing against a symbol
literal, the question is moot:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> ,optimize (equal? 'x s)
$14 = (eq? 'x s)
--8<---------------cut here---------------end--------------->8---

Ludo’.

diff --git a/libguile/eq.c b/libguile/eq.c
index 627d6f09b..16c5bfb3f 100644
--- a/libguile/eq.c
+++ b/libguile/eq.c
@@ -303,6 +303,8 @@ scm_equal_p (SCM x, SCM y)
     return SCM_BOOL_F;
   if (SCM_IMP (y))
     return SCM_BOOL_F;
+  if (scm_is_symbol (x) || scm_is_symbol (y))
+    return SCM_BOOL_F;
   if (scm_is_pair (x) && scm_is_pair (y))
     {
       if (scm_is_false (scm_equal_p (SCM_CAR (x), SCM_CAR (y))))