Subject: Re: bug#56682: locked narrowing

From: Gregory Heytings <[email-masked]>
Date: Sat, 26 Nov 2022 22:42:35 +0000

>
> I actually think these should not be called with-SOMETHING, since they 
> aren't macros.  And yes, the function names should include "--".
>

Done, I went for internal--with-narrowing.

>> Because 'pop' on an empty stack is an error
>
> You don't need to pop, just return without doing anything.
>
>> and (more importantly) it would mean that the narrowing_locks alist is 
>> corrupted.  The cdr's in that alist should never be nil.
>
> A comment to that effect should be in order, then.
>

Sorry, I was confused, in fact just return'ing in that case is fine.

>> 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.
>
> I don't think I understand (you avoid the assertion in other cases which 
> look similar to me), but if there are good reasons, please add comments 
> there explaining the rationale.
>

I'm just being extra cautious here.  I added a comment to explain that 
assertion (and the previous one).

>
> We can add the tests after the branch is cut, I just wanted to be sure 
> we will have them eventually.
>

I'll do that.