Subject: Re: bug#56682: locked narrowing

From: Gregory Heytings <[email-masked]>
Date: Sat, 26 Nov 2022 16:15:34 +0000

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.