From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#56682: locked narrowing Date: Sat, 26 Nov 2022 12:08:38 +0200 Message-ID: <83fse6t4d5.fsf@gnu.org> References: <831qtgff78.fsf@gnu.org> <83zgg4dw4y.fsf@gnu.org> <83r11gdrr4.fsf@gnu.org> <83edxfds7s.fsf@gnu.org> <83r11fc80o.fsf@gnu.org> <83o7wjc6o2.fsf@gnu.org> <83lernc5gu.fsf@gnu.org> <83k076dd7d.fsf@gnu.org> <83czcyd8jf.fsf@gnu.org> <83a682d66r.fsf@gnu.org> <837d36ceno.fsf@gnu.org> <37dd2827f54f8bbda5e3@heytings.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="29829"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 56682@debbugs.gnu.org, monnier@iro.umontreal.ca, dgutov@yandex.ru To: Gregory Heytings Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Nov 26 11:09:21 2022 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 1oys7d-0007Zz-65 for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 26 Nov 2022 11:09:21 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oys7N-0004C6-96; Sat, 26 Nov 2022 05:09:05 -0500 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 1oys7K-0004Bw-Mr for bug-gnu-emacs@gnu.org; Sat, 26 Nov 2022 05:09:03 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oys7K-00047y-92 for bug-gnu-emacs@gnu.org; Sat, 26 Nov 2022 05:09:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oys7J-0003uq-NF for bug-gnu-emacs@gnu.org; Sat, 26 Nov 2022 05:09:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 26 Nov 2022 10:09:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 56682 X-GNU-PR-Package: emacs Original-Received: via spool by 56682-submit@debbugs.gnu.org id=B56682.166945730915014 (code B ref 56682); Sat, 26 Nov 2022 10:09:01 +0000 Original-Received: (at 56682) by debbugs.gnu.org; 26 Nov 2022 10:08:29 +0000 Original-Received: from localhost ([127.0.0.1]:37864 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oys6m-0003u6-L0 for submit@debbugs.gnu.org; Sat, 26 Nov 2022 05:08:29 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:36572) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oys6i-0003to-En for 56682@debbugs.gnu.org; Sat, 26 Nov 2022 05:08:27 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oys6a-00044U-Fx; Sat, 26 Nov 2022 05:08:16 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=ltVT53kuuAI87WkqR9baPpnEx8y1nRLpvcyKomKG930=; b=DH2DBxdKxP1X SivCy8U8NNfoBVQfxxEjWnFDQZxlqj92vlGGAIzBT1P9YKFpnVwlzGmKjR3evYLefzlWdSmuvtHtQ lZwB+oCbZ5fYZVGpZf/3HdK9iRQhRnaLCkXzeb3i6E/Gb2HaFoO9hUIBUyEO1PGqpGnpjgHrDJy25 xE4eN27Q+nGxN9i+w+cCjFS50kBXpYizei95zw6Pu9aBdDS1n9YW5VynXm43XOgtBVFD9Rj5ae2ea noCqQ+ID0G2VgsNn/eXMWVWiS6XKGYF5OVrweHMGCZhBOsCylSx18wsEVKcPxoXfoPiwX3br/+Wmz kPdsxcYuycK719g0RvMHWg==; Original-Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oys6Y-0005Wq-BQ; Sat, 26 Nov 2022 05:08:15 -0500 In-Reply-To: (message from Gregory Heytings on Sat, 26 Nov 2022 00:30:53 +0000) 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:249063 Archived-At: > Date: Sat, 26 Nov 2022 00:30:53 +0000 > From: Gregory Heytings > cc: 56682@debbugs.gnu.org, dgutov@yandex.ru > > > Hi Eli & Stefan, > > I finally found the time to work on this again. I pushed a number of > further improvements in the feature/improved-locked-narrowing branch. > > In particular, Stefan, I used the suggestions you made in > jwvtu623mi9.fsf-monnier+emacs@gnu.org on August 23rd (I can't believe so > much time has passed!), and added comments to make the code (hopefully) > easier to read. > > Comments and feedback welcome. Below: > +(defun with-narrowing-1 (start end tag body) > + "Helper function for `with-narrowing', which see." > + (save-restriction > + (progn > + (narrow-to-region start end) > + (narrowing-lock tag) > + (funcall body)))) > + > +(defun with-narrowing-2 (start end body) > + "Helper function for `with-narrowing', which see." > + (save-restriction > + (progn > + (narrow-to-region start end) > + (funcall body)))) > + Should these two helpers be internal functions? > +/* Retrieve one of the BEGV/ZV bounds of a narrowing in BUF from the > + narrowing_locks alist. When OUTERMOST is true, the bounds that > + were set by the user and that are visible on display are returned. > + Otherwise the innermost locked narrowing bounds are returned. */ > +static ptrdiff_t > +narrowing_lock_get_bound (Lisp_Object buf, bool begv, bool outermost) > +{ > + if (NILP (Fbuffer_live_p (buf))) > + return 0; Zero is a strange value to return from a function the is documented to return buffer positions. It is also not documented in the commentary. Also, I'm not sure I understand why we need the buffer-live-p test here. This is an internal C subroutine, so it's up to the callers to make sure BUF is a live buffer. > + buffer_locks = Fcar (Fcdr (buffer_locks)); Please avoid calling Fcar and Fcdr from C -- that just burns up CPU cycles for no good reason. Use CAR and CDR instead. In general, if some Lisp primitive has a subroutine that is specifically for calls from C, always prefer to use the subroutine, since that's why those subroutines are there in the first place. They typically avoid some safeguards and checks that are necessary when calling from Lisp, but not when calling from C. > + Lisp_Object marker = begv ? Fcar (bounds) : Fcar (Fcdr (bounds)); > + eassert (MARKERP (marker)); > + Lisp_Object pos = Fmarker_position (marker); > + eassert (! NILP (pos)); Why isn't there an assertion that the marker belongs to the buffer BUF? I think this is a more probable problem than position being nil. > +/* Remove the innermost lock in BUF from the narrowing_lock alist. */ > static void > -unwind_locked_zv (Lisp_Object point_max) > +narrowing_lock_pop (Lisp_Object buf) > { > - SET_BUF_ZV (current_buffer, XFIXNUM (point_max)); > + Lisp_Object buffer_locks = assq_no_quit (buf, narrowing_locks); > + eassert (! NILP (buffer_locks)); Why this assertion? There's no similar assertions in other functions that deal with narrowing_locks. > + if (EQ (narrowing_lock_peek_tag (buf), Qoutermost_narrowing)) > + narrowing_locks = Fdelq (Fassoc (buf, narrowing_locks, Qnil), > + narrowing_locks); > + else > + Fsetcdr (buffer_locks, list1 (Fcdr (Fcar (Fcdr (buffer_locks))))); Please use XSETCDR instead of Fsetcdr. > + begv = narrowing_lock_get_bound (buf, true, false); > + zv = narrowing_lock_get_bound (buf, false, false); > + if (begv && zv) This goes back to the question about returning zero for buffer positions: zero is an unusual value, I'd prefer to use -1 instead. > { > - EMACS_INT tem = s; s = e; e = tem; > + SET_BUF_BEGV (XBUFFER (buf), begv); > + SET_BUF_ZV (XBUFFER (buf), zv); The values you keep in narrowing_locks are markers, so they include the byte position. By returning only the character position, you've lost that useful information, and thus SET_BUF_BEGV/SET_BUF_ZV will have to recompute it. I think it's better to return the marker there, and then you can use the byte positions here and call SET_BUF_BEGV_BOTH and SET_BUF_ZV_BOTH. > +/* When redisplay is called in a function executed while a locked > + narrowing is in effect, restore the narrowing bounds that were set > + by the user, and restore the bounds of the locked narrowing when > + returning from redisplay. */ > +void > +reset_outermost_narrowings (void) This function doesn't care about redisplay, it will do its job for any caller. So the commentary should describe what it does regardless of redisplay, and then say that it is called from redisplay when so-and-so. > + for (val = narrowing_locks; CONSP (val); val = XCDR (val)) > { > - if (!(BEGV <= s && s <= e && e <= ZV)) > - args_out_of_range (start, end); > + buf = Fcar (Fcar (val)); > + eassert (BUFFERP (buf)); > + ptrdiff_t begv = narrowing_lock_get_bound (buf, true, true); > + ptrdiff_t zv = narrowing_lock_get_bound (buf, false, true); > + SET_BUF_BEGV (XBUFFER (buf), begv); > + SET_BUF_ZV (XBUFFER (buf), zv); No checking of values of begv and zv? > + record_unwind_protect (unwind_reset_outermost_narrowing, buf); > + } > +} > > - if (BEGV != s || ZV != e) > - current_buffer->clip_changed = 1; > +/* Helper functions to save and restore the narrowing locks of the > + current buffer in save-restriction. */ > +static Lisp_Object > +narrowing_locks_save (void) > +{ > + Lisp_Object buf = Fcurrent_buffer (); > + Lisp_Object locks = assq_no_quit (buf, narrowing_locks); > + if (NILP (locks)) > + return Qnil; > + locks = Fcar (Fcdr (locks)); > + return Fcons (buf, Fcopy_sequence (locks)); > +} > + > +static void > +narrowing_locks_restore (Lisp_Object buf_and_saved_locks) > +{ > + if (NILP (buf_and_saved_locks)) > + return; > + Lisp_Object buf = Fcar (buf_and_saved_locks); > + eassert (BUFFERP (buf)); > + Lisp_Object saved_locks = Fcdr (buf_and_saved_locks); > + eassert (! NILP (saved_locks)); Again, I don't understand the need for an assertion here. Just return if saved_locks is nil. What about some tests? Thanks.