From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.bugs Subject: bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled. Date: Sun, 13 Aug 2023 15:27:26 +0000 Message-ID: References: <83msywg5a0.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="32358"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 65051@debbugs.gnu.org, acm@muc.de, Eli Zaretskii To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Aug 13 17:28:13 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qVD0m-0008Gr-K7 for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 13 Aug 2023 17:28:12 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qVD0e-0000oi-In; Sun, 13 Aug 2023 11:28:04 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qVD0c-0000oY-Ow for bug-gnu-emacs@gnu.org; Sun, 13 Aug 2023 11:28:02 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qVD0c-0006Oa-GX for bug-gnu-emacs@gnu.org; Sun, 13 Aug 2023 11:28:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qVD0b-0004XG-SJ for bug-gnu-emacs@gnu.org; Sun, 13 Aug 2023 11:28:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alan Mackenzie Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 13 Aug 2023 15:28:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 65051 X-GNU-PR-Package: emacs Original-Received: via spool by 65051-submit@debbugs.gnu.org id=B65051.169194045517401 (code B ref 65051); Sun, 13 Aug 2023 15:28:01 +0000 Original-Received: (at 65051) by debbugs.gnu.org; 13 Aug 2023 15:27:35 +0000 Original-Received: from localhost ([127.0.0.1]:60103 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qVD0B-0004Wb-9h for submit@debbugs.gnu.org; Sun, 13 Aug 2023 11:27:35 -0400 Original-Received: from mx3.muc.de ([193.149.48.5]:40022) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qVD09-0004WO-G1 for 65051@debbugs.gnu.org; Sun, 13 Aug 2023 11:27:34 -0400 Original-Received: (qmail 2692 invoked by uid 3782); 13 Aug 2023 17:27:27 +0200 Original-Received: from acm.muc.de (p4fe152a2.dip0.t-ipconnect.de [79.225.82.162]) (using STARTTLS) by colin.muc.de (tmda-ofmipd) with ESMTP; Sun, 13 Aug 2023 17:27:27 +0200 Original-Received: (qmail 23192 invoked by uid 1000); 13 Aug 2023 15:27:26 -0000 Content-Disposition: inline In-Reply-To: X-Submission-Agent: TMDA/1.3.x (Ph3nix) X-Primary-Address: acm@muc.de X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:267368 Archived-At: Hello, Stefan. I haven't changed my view that the current handling of SWPs in equal is a bug, and that the bug should be fixed. Your patch isn't this fix. I continue to be uneasy about the contradictions in your attidude where, on the one hand, you say "anything we do here sucks equally" and describe this conversation as "bikeshedding", and on the other hand you're steamrolling over my clear vision for fixing the bug. Nevertheless, here are my comments on your proposed patch, as promised. On Sat, Aug 12, 2023 at 14:46:24 -0400, Stefan Monnier wrote: > Thanks, > Stefan > diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi > index 34db0caf3a8..d2e397faf80 100644 > --- a/doc/lispref/symbols.texi > +++ b/doc/lispref/symbols.texi > @@ -782,11 +782,16 @@ Symbols with Position > @cindex symbol with position > @cindex bare symbol > -A @dfn{symbol with position} is a symbol, the @dfn{bare symbol}, > -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. > +A @dfn{symbol with position} is a pair of a symbol, the @dfn{bare > +symbol}, together with an unsigned integer called the @dfn{position}. > +Symbol with position cannot themselves have entries in obarrays > +(contrary to their bare symbols; @pxref{Creating Symbols}). "Cannot" is inappropriate here, since there is nothing regrettable about SWPs not being stored in obarrays. "Contrary" is also inappropriate since there is no disagreement, opposition, or conflict between the two things. "As contrasted with" would be better. On the other hand, why didn't you just leave it as I had left it? > + > +Symbols with position are for the use of the byte compiler, which > +records in them the position of each symbol occurrence and uses those > +positions in warning and error messages. They shouldn't normally be > +used otherwise. Doing so can cause unexpected results with basic > +Emacs functions such as @code{eq} and @code{equal}. > The printed representation of a symbol with position uses the hash > notation outlined in @ref{Printed Representation}. It looks like > @@ -798,11 +803,21 @@ Symbols with Position > For most purposes, when the flag variable > @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with > -positions behave just as bare symbols do. For example, @samp{(eq > -# foo)} has a value @code{t} when that variable > -is set (but @code{nil} when it isn't set). Most of the time in Emacs this > -variable is @code{nil}, but the byte compiler binds it to @code{t} > -when it runs. > +position behave just as their bare symbols would. For example, > +@samp{(eq # foo)} has a value @code{t} when the > +variable is set; likewise, @code{equal} will treat a symbol with > +position argument as its bare symbol. This is suboptimal for your version. The paragraph is about Emacs's behaviour when symbols-with-pos-enabled is non-nil. You're including behaviour in this paragraph which you want to be independent of s-w-p-enabled. It really needs its own paragraph. > + > +When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with > +position continue to exist, but do not always behave as symbols. The "do not always" is vaguer than it should be. The doc should be explicit about when SWPs behave as symbols, and when not. > +Most importantly @code{eq} only returns @code{t} when given truly > +identical arguments, for performance reasons. .... There's more to it than "for performance reasons". I think you could omit ", for performance reasons" since it is more likely to cause confusion than anything else. > .... @code{equal} on the > +other hand continues to treat a symbol with > +position argument as its bare symbol. "Continues to" is inappropriate here, since there is nothing continuous happening, nor a continuous "treating" of anything. A useful phrase might be "regardless of the value of @code{symbols-with-pos-enabled}". But as noted above for s-w-p-e's non-nil case, this doesn't belong in the paragraph about the behaviour when s-w-p-enabled is nil. > + > +Most of the time in Emacs @code{symbols-with-pos-enabled} is > +@code{nil}, but the byte compiler and the native compiler bind it to > +@code{t} when they run. > Typically, symbols with position are created by the byte compiler > calling the reader function @code{read-positioning-symbols} What's missing: (i) You'll need to amend the definition of symbols-with-pos-enabled in its @defvar, since you're changing it's meaning. (ii) You should document the behaviour of SWP's in the sections on `eq' and `equal'. (iii) See below. > diff --git a/src/fns.c b/src/fns.c > index ac30670b3ac..fde4ef6b08b 100644 > --- a/src/fns.c > +++ b/src/fns.c > @@ -5166,7 +5166,7 @@ sxhash_obj (Lisp_Object obj, int depth) > hash = sxhash_combine (hash, sxhash_obj (XOVERLAY (obj)->plist, depth)); > return SXHASH_REDUCE (hash); > } > - else if (symbols_with_pos_enabled && pvec_type == PVEC_SYMBOL_WITH_POS) > + else if (pvec_type == PVEC_SYMBOL_WITH_POS) > return sxhash_obj (XSYMBOL_WITH_POS (obj)->sym, depth + 1); > else > /* Others are 'equal' if they are 'eq', so take their Have you tested this thoroughly? Hash tables were one of the more troublesome things to get right when I was developing this stuff. It also needs documenting in the hash table chapter of the elisp manual. -- Alan Mackenzie (Nuremberg, Germany).