Subject: Re: locked narrowing (was: bug#56682: Fix the long lines font locking related slowdowns)

From: Gregory Heytings <[email-masked]>
Date: Tue, 16 Aug 2022 19:39:21 +0000

>
> [ Sorry, Gregory.  But see below for more concrete/constructive 
> criticism. ]
>

There's no need to be sorry.  This is WIP, any (constructive) criticism is 
welcome.  And I do not understand why you did not raise some of the points 
below earlier.  Perhaps I misuderstood or overlooked something in what you 
said earlier, but to me your objection to that part of the code until now 
was mainly/purely "philosophical".

>
> - Not only you can't `widen` but you can't narrow either 
> (`narrow-to-region` just does nothing when the restriction is locked, 
> regardless if the call tries to grow or shrink the restriction). AFAIK 
> this is a plain bug, but given the fundamental disagreement about what 
> is "right" in this discussion, I can't be sure.
>

This (allowing narrow-to-region and widen to work within the bounds of the 
locked narrowing) is currently being implemented, after an off-list 
discussion with Alan.

>
> - The locking is global: any `widen` or `narrow-to-region` will fail 
> when a locked narrowing is in effect, even when it's applied to an 
> unrelated buffer.  Again, it looks like a plain bug, but...
>

It's not global, given that it is controlled by symbol that is bound 
within a function called inside a specific buffer.  But indeed, if inside 
that function we use narrow-to-region/widen in another buffer, that would 
fail.  It seems easy to correct that bug, though: if 
(current_buffer->long_line_optimizations_p && ! NILP 
(Vrestrictions_locked)) ...

>
> - The doc for `restrictions-locked` says:
>
>  This happens when `narrow-to-region', which see, is called from Lisp
>  with an optional argument LOCK non-nil.
>
>  but the `narrow-to-region` function doesn't have any such LOCK 
> argument.
>

That should have been removed, indeed.  In a previous iteration of the 
code a LOCK argument was added to that function, and later removed.

>
> - The `narrow_to_region_internal` function, when called with `lock=true` 
> oddly includes the functionality of save-restriction to restore the 
> restriction and point but not to restore the `restrictions-locked` 
> state.
>

This I do not understand.  The comment above that function explains that 
it should be followed by "specbind (Qrestrictions_locked, Qt)".  There is 
no need to restore the restrictions-locked state, or am I misunderstanding 
something?

>
> - In that same function the two branches of the `if (lock)` have a lot 
> of obvious redundancy.
>

Not "a lot", but indeed there's some redundancy there that could have been 
avoided.

I'll fix those problems soon.