bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Mattias Engdegård]

6 aug. 2023 kl. 17.02 skrev Alan Mackenzie <acm@muc.de>:

>>> together with an unsigned integer called the @dfn{position}.  These
>>> -objects are intended for use by the byte compiler, which records in
>>> -them the position of each symbol occurrence and uses those positions
>>> -in warning and error messages.
>>> +objects are stored internally much like vectors
> 
>> Not sure why we want to say how they are stored here. They can be
>> stored in bubble memory for all the user cares.
> 
> The point is, they are _not_ stored in the obarray.  Eli specifically
> asked me to clarify this point, yesterday.

Oh that part is perfectly fine (thank you), we just don't need to say that the 
sympos objects are stored "like vectors" -- that just confuses the reader.

>>> +When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
>>> +position continue to exist, but do not behave as symbols, or have the
>>> +other useful properties outlined in the previous paragraph.  @code{eq}
>>> +returns @code{t} when given identical arguments, and @code{equal}
>>> +returns @code{t} when given arguments with @code{equal} components.
> 
>> Since the components are bare symbols and fixnums, equality and
>> identity for them are equivalent, right?
> 
> No.  If there are two distinct SWPs with the same bare symbol and the
> same position, they should be equal, but not eq.  But the real point is
> to contrast how equal and eq work when symbols-with-pos-enabled is nil
> with when it is non-nil.

I meant that the components of equal sympos objects aren't merely equal but 
identical. (This is a very minor quibble; you can keep the text if you like.)

>> OK. This reduces the number of branches in the hot path for ordinary
>> (non-sympos) code by one while adding one to sym-pos code, and that
>> should be a fair trade-off. The new branch should be well-predicted but
>> is still consuming resources.
> 
> I did some simple timings on the old and new code, and the new code is
> not slower.

This is not easy to measure and details matter, but as I said -- there is no 
reason to believe that your changes should be a regression in the important 
measure, rather the opposite.

>>> +   if (SYMBOL_WITH_POS_P(o1)) /* symbols_with_pos_enabled is false. */
>>> +     return (internal_equal (XSYMBOL_WITH_POS (o1)->sym,
>>> +                             XSYMBOL_WITH_POS (o2)->sym,
>>> +                             equal_kind, depth + 1, ht)
>>> +             && internal_equal (XSYMBOL_WITH_POS (o1)->pos,
>>> +                                XSYMBOL_WITH_POS (o2)->pos,
>>> +                                equal_kind, depth + 1, ht));
> 
>> Why recurse here if the components are a bare symbol and a fixnum,
>> respectively?
> 
> Maybe in case they might somehow be something else?

No, we must be able to assume that internal invariants hold when we offer no 
way for them to be violated. Let's just change the calls to BASE_EQ and be done 
with it.

>> However we should make an effort to prevent the compiler from
>> optimising (eq X X) -> t etc, which it is completely entitled to doing,
>> ....
> 
> Why?  (eq X X) is t in all circumstances, whether X is a symbol, a cons
> structure, or anything else.  What am I missing, here?

If the compiler transforms (eq foo1 foo1) into t then the test won't actually 
exercise the implementation of `eq`.

>> .... and also test both the interpreted and compiled version of `eq`
>> and `equal`.
> 
> They're the same code in both cases.  I'm missing something here, too, I
> think.

Byte-code doesn't call Feq, it uses its own implementation. They should work 
identically but as we are checking edge cases here we'd better be sure about 
that.

>> The test bytecomp--eq-symbols-with-pos-enabled already does most of
>> this for a different reason. Perhaps it can be extended to cover
>> `equal` as well?
> 
> I don't have such a test in my repository anywhere.  Are you sure you
> wrote it right?

It was added in 44d7fd3805.