From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: feature/improved-locked-narrowing 3bf19c417f: Merge master into feature/improved-locked-narrowing. Date: Tue, 23 Aug 2022 14:33:13 -0400 Message-ID: References: <166126990067.22556.5470565968968942153@vcs2.savannah.gnu.org> <20220823155144.937DDC0088A@vcs2.savannah.gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="28588"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: To: Gregory Heytings Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Aug 23 20:42:57 2022 Return-path: Envelope-to: ged-emacs-devel@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 1oQYrY-00078r-8i for ged-emacs-devel@m.gmane-mx.org; Tue, 23 Aug 2022 20:42:56 +0200 Original-Received: from localhost ([::1]:40358 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oQYrW-00010j-5U for ged-emacs-devel@m.gmane-mx.org; Tue, 23 Aug 2022 14:42:55 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:39340) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oQYib-0001Ka-8z for emacs-devel@gnu.org; Tue, 23 Aug 2022 14:33:42 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:58365) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oQYiE-0004hJ-U2 for emacs-devel@gnu.org; Tue, 23 Aug 2022 14:33:21 -0400 Original-Received: from pmg1.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id D3B30100138; Tue, 23 Aug 2022 14:33:16 -0400 (EDT) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id A196C1000FB; Tue, 23 Aug 2022 14:33:14 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1661279594; bh=9ZmzehFxW+lA1m6Nbfw5Ecnx7Xayp6bMLi5pvUYcUTk=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=NnhXr0gRs+UNCa3crEvsud3Z8pGmgP7E5wZnCoEjy8i0r8UFmkKjGjZnbkEdbU6m7 3QJ/LmBrSTEFqhP7yrgIXtCo0sZPUd0wKlSssJoN/0t0VaF4QpGKBSZSAVnn15MVKg RhpI3/SMrM1p+YIUEcdrd9SML/hvhY0fXvYaUtfLuIqS6mXufdWd6nw+uA9a55FDFH QbF85DB54zD4Ksou9W7CYJFgz0BiVcnG81y6QJUD6Ff4GsBFNG2CQl4CAxIhUYvUMX w/T0chCoXkg8WUqRURsg0sVE4YVdXrPm0dTrQcrosO8qDDEZRF5ZzYhOSm2U7xSQqa zYws30k9hkxHg== Original-Received: from alfajor (unknown [45.44.229.252]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 7AE711204B2; Tue, 23 Aug 2022 14:33:14 -0400 (EDT) In-Reply-To: <20220823155144.937DDC0088A@vcs2.savannah.gnu.org> (Gregory Heytings's message of "Tue, 23 Aug 2022 11:51:41 -0400 (EDT)") Received-SPF: pass client-ip=132.204.25.50; envelope-from=monnier@iro.umontreal.ca; helo=mailscanner.iro.umontreal.ca X-Spam_score_int: -42 X-Spam_score: -4.3 X-Spam_bar: ---- X-Spam_report: (-4.3 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:293914 Archived-At: Hi Gregory, Here are some comments about the code on this branch. > +(defmacro with-locked-narrowing (start end tag &rest body) > + "Execute BODY with restrictions set to START and END and locked with TAG. > + > +Inside BODY, `narrow-to-region' and `widen' can be used only > +within the START and END limits, unless the restrictions are > +unlocked by calling `narrowing-unlock' with TAG. See > +`narrowing-lock' for a more detailed description. The current > +restrictions, if any, are restored upon return." > + `(save-restriction > + (unwind-protect > + (progn > + (narrow-to-region ,start ,end) > + (narrowing-lock ,tag) > + ,@body) > + (narrowing-unlock ,tag) > + (widen)))) I think the final is redundant because of the surrounding `save-restriction`. Based on past experience, I recommend you split it into a macro and a function, where the function does the bulk of the work. I.e. (defmacro with-locked-narrowing (start end tag &rest body) ... `(with-locked-narrowing-1 ,start ,end ,tag (lambda () ,@body))) (defun with-locked-narrowing-1 (start end tag body-fun) ...) Also, I'd suggest you rename this to `with-narrowing` and make `tag` optional (e.g. by providing it with a `:locked ` at the beginning of `body`) such that if it's absent/nil then we skip the lock/unlock. BTW, maybe the `narrowing-unlock` at the end should be made redundant as well by making `save-restriction` automatically save&restore the locks? > DEFUN ("widen", Fwiden, Swiden, 0, 0, "", > doc: /* Remove restrictions (narrowing) from current buffer. > -This allows the buffer's full text to be seen and edited. > > -Note that, when the current buffer contains one or more lines whose > -length is above `long-line-threshold', Emacs may decide to leave, for > -performance reasons, the accessible portion of the buffer unchanged > -after this function is called from low-level hooks, such as > -`jit-lock-functions' or `post-command-hook'. */) > +This allows the buffer's full text to be seen and edited, unless > +restrictions have been locked with `narrowing-lock', which see, in > +which case the restrictions that were current when `narrowing-lock' > +was called are restored. */) The doc should be more explicit about what happens when there's a lock (it currently only says that it does not allow to see the full text but doesn't say if it just does nothing or if it widens to the locked narrowing or what). > (void) > { > - if (! NILP (Vrestrictions_locked)) > - return Qnil; > - if (BEG != BEGV || Z != ZV) > - current_buffer->clip_changed = 1; > - BEGV = BEG; > - BEGV_BYTE = BEG_BYTE; > - SET_BUF_ZV_BOTH (current_buffer, Z, Z_BYTE); > + Fset (Qoutermost_narrowing, Qnil); I must say, I still haven't understood how `Qoutermost_narrowing` and `Qnarrowing_locks` work/interact, so I suggest you document their contents more fully (either in their docstring or in comments next to it). > + if (NILP (Vnarrowing_locks)) > + { > + if (BEG != BEGV || Z != ZV) > + current_buffer->clip_changed = 1; > + BEGV = BEG; > + BEGV_BYTE = BEG_BYTE; > + SET_BUF_ZV_BOTH (current_buffer, Z, Z_BYTE); > + } > + else > + { > + ptrdiff_t begv = XFIXNUM (Fcar (Fcdr (Fcar (Vnarrowing_locks)))); > + ptrdiff_t zv = XFIXNUM (Fcdr (Fcdr (Fcar (Vnarrowing_locks)))); Since you use XFIXNUM (i.e. you assume that `Vnarrowing_locks` has a particular shape), you can also use XCAR/XCDR here. > + if (begv != BEGV || zv != ZV) > + current_buffer->clip_changed = 1; > + SET_BUF_BEGV (current_buffer, begv); > + SET_BUF_ZV (current_buffer, zv); > + if (EQ (Fcar (Fcar (Vnarrowing_locks)), Qoutermost_narrowing)) > + Fset (Qnarrowing_locks, Qnil); I think you can `Vnarrowing_locks = Qnil` here. But I must admit I don't really understand why you're messing with narrowing_locks here. > +DEFUN ("narrow-to-region", Fnarrow_to_region, Snarrow_to_region, 2, 2, "r", > + doc: /* Restrict editing in this buffer to the current region. > +The rest of the text becomes temporarily invisible and untouchable > +but is not deleted; if you save the buffer in a file, the invisible > +text is included in the file. \\[widen] makes all visible again. > +See also `save-restriction'. > > +When calling from Lisp, pass two arguments START and END: > +positions (integers or markers) bounding the text that should > +remain visible. > > +When restrictions have been locked with `narrowing-lock', which see, > +`narrow-to-region' can be used only within the limits of the > +restrictions that were current when `narrowing-lock' was called. If > +the START or END arguments are outside these limits, the corresponding > +limit of the locked restriction is used instead of the argument. */) > + (Lisp_Object start, Lisp_Object end) > { > EMACS_INT s = fix_position (start), e = fix_position (end); > > @@ -2731,35 +2743,29 @@ narrow_to_region_internal (Lisp_Object start, Lisp_Object end, bool lock) > EMACS_INT tem = s; s = e; e = tem; > } > > + if (!(BEG <= s && s <= e && e <= Z)) > + args_out_of_range (start, end); > > + if (! NILP (Vnarrowing_locks)) > { > + ptrdiff_t begv = XFIXNUM (Fcar (Fcdr (Fcar (Vnarrowing_locks)))); > + ptrdiff_t zv = XFIXNUM (Fcdr (Fcdr (Fcar (Vnarrowing_locks)))); > + if (s < begv) s = begv; > + if (s > zv) s = zv; > + if (e < begv) e = begv; > + if (e > zv) e = zv; > + } Maybe the `args_out_of_range` error should be signaled based on `begv/zv` (when inside a lock) rather than `BEG/Z` (i.e. if trying to `narrow-to-region` beyond the bounds of the current lock we'd get an error rather than silently using the current lock's limit). BTW, using XFIXNUM is wrong because the bounds need to be markers, to account for possible buffer modifications (you could argue it's only needed for the ZV part and not for BEGV because you can go before the locked narrowing without undoing this lock, but it's still possible to insert/remove text before the beginning of the current locked narrowing by using an indirect buffer). > + Fset (Qoutermost_narrowing, > + Fcons (Fcons (Qoutermost_narrowing, > + Fcons (make_fixnum (BEGV), make_fixnum (ZV))), > + Qnil)); This needs explaining. > +DEFUN ("narrowing-lock", Fnarrowing_lock, Snarrowing_lock, 1, 1, 0, [...] > + (Lisp_Object tag) > +{ > + if (NILP (Vnarrowing_locks)) > + Fset (Qnarrowing_locks, Voutermost_narrowing); This also. > + Fset (Qnarrowing_locks, > + Fcons (Fcons (tag, Fcons (make_fixnum (BEGV), make_fixnum (ZV))), > + Vnarrowing_locks)); > + return Qnil; > +} As mentioned, these saved positions need to be markers. > +DEFUN ("narrowing-unlock", Fnarrowing_unlock, Snarrowing_unlock, 1, 1, 0, > + doc: /* Unlock a narrowing locked with (narrowing-lock TAG). > + > +Unlocking restrictions locked with `narrowing-lock' should be used > +sparingly, after carefully considering the reasons why restrictions > +were locked. Restrictions are typically locked around portions of > +code that would become too slow, and make Emacs unresponsive, if they > +were executed in a large buffer. For example, restrictions are locked > +by Emacs around low-level hooks such as `fontification-functions' or > +`post-command-hook'. */) > + (Lisp_Object tag) > +{ > + if (EQ (Fcar (Fcar (Vnarrowing_locks)), tag)) > + Fset (Qnarrowing_locks, Fcdr (Vnarrowing_locks)); > + return Qnil; > +} This allows removing a lock once and for all, but for example in `nlinum` I'd like to just locally/temporarily escape the lock installed by the long-lines optimization, but restore the lock when I'm done. I don't think `narrowing-unlock` lets me do that yet currently. It would if `save-restriction` were to save&restore `narrowing_locks`, tho, so I could do: (save-restriction (narrowing-unlock 'long-lines) (narrowing-unlock 'mmm) (widen) ... compute current line number) Also, I can imagine situations where one might like to escape from a set of locks (e.g. the ones installed by long-lines and by mmm-mode) without knowing in which order they may have been installed, so maybe when overriding a lock we should be ale to provide a set (list) of tags rather than a single tag. Furthermore, I suspect that all such temporary override of the lock are combined with `widen`, so maybe instead of a new function, we should just add a `tags` arg to `widen`, so nlinum would do: (save-restriction (widen '(long-lines mmm)) ... compute current line number) > +static void > +unwind_narrow_to_region_locked (Lisp_Object tag) > +{ > + Fnarrowing_unlock (tag); > + Fwiden (); > +} > > +void > +narrow_to_region_locked (Lisp_Object begv, Lisp_Object zv, Lisp_Object tag) > { > + Fnarrow_to_region (begv, zv); > + Fnarrowing_lock (tag); > + record_unwind_protect (restore_point_unwind, Fpoint_marker ()); > + record_unwind_protect (unwind_narrow_to_region_locked, tag); > } This seems to duplicate the code of `with-locked-narrowing` (just written in C this time). > + DEFSYM (Qnarrowing_locks, "narrowing-locks"); > + DEFVAR_LISP ("narrowing-locks", Vnarrowing_locks, > + doc: /* List of narrowing locks in the current buffer. Internal use only. */); > + Vnarrowing_locks = Qnil; > + Fmake_variable_buffer_local (Qnarrowing_locks); > + Funintern (Qnarrowing_locks, Qnil); At the very least for debugging purposes it would be very convenient to have access to this var from ELisp, so I wouldn't unintern it (but I'd give it a double hyphen in the name). > + DEFSYM (Qoutermost_narrowing, "outermost-narrowing"); > + DEFVAR_LISP ("outermost-narrowing", Voutermost_narrowing, > + doc: /* Outermost narrowing bounds, if any. Internal use only. */); > + Voutermost_narrowing = Qnil; > + Fmake_variable_buffer_local (Qoutermost_narrowing); > + Funintern (Qoutermost_narrowing, Qnil); As mentioned, I don't understand what this is about, so I don't have good comments to make about the rest of the code which touches this. Stefan