Package: emacs;
Reported by: Gregory Heytings <gregory <at> heytings.org>
Date: Thu, 21 Jul 2022 18:01:01 UTC
Severity: normal
Done: Gregory Heytings <gregory <at> heytings.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Eli Zaretskii <eliz <at> gnu.org> To: Gregory Heytings <gregory <at> heytings.org> Cc: 56682 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca, dgutov <at> yandex.ru Subject: bug#56682: locked narrowing Date: Sat, 26 Nov 2022 12:08:38 +0200
> Date: Sat, 26 Nov 2022 00:30:53 +0000
> From: Gregory Heytings <gregory <at> heytings.org>
> cc: 56682 <at> debbugs.gnu.org, dgutov <at> 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 <at> 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.