bug#56682: locked narrowing

[Gregory Heytings]

Thanks for your detailed review, and for the useful suggestions!

A few comments/questions below:

> > +(defun with-narrowing-1 (start end tag body)
> > +(defun with-narrowing-2 (start end body)
>
> Should these two helpers be internal functions?

You mean, they should be called with-narrowing--{1,2} (or perhaps 
with--narrowing-{1,2})?

> > +/* 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.

Because 'pop' on an empty stack is an error, and (more importantly) it 
would mean that the narrowing_locks alist is corrupted.  The cdr's in that 
alist should never be nil.

> > +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.

Strictly speaking there is no need for an assertion, but again it would 
mean that something that isn't supposed to happen happened.  In this case 
it would mean that narrowing_locks_restore is called with a list of length 
1, containing only a buffer, whereas it is supposed to be called with the 
return value of narrowing_locks_save, which is either nil or a list of 
length >= 2.

> 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.

Thanks, I didn't think of that possibility, it seems better indeed.

> What about some tests?

I'd have to add some tests, indeed.  Not sure I'll have time to do that 
today.