bug#56682: feature/improved-locked-narrowing 9dee6df39c: Reworked locked narrowing.

[Gregory Heytings]

Thanks for your detailed review.

> > +      struct Lisp_Marker *begv
> > +       = labeled_restrictions_get_bound (buf, true, true);
> > +      struct Lisp_Marker *zv
> > +       = labeled_restrictions_get_bound (buf, false, true);
>
> Why the strange design of having a function return a pointer to a 
> 'struct Lisp_Marker'? why not return the marker itself instead?  (I 
> realize that this was so in the code we already have, but I still don't 
> understand why you did it that way, and prefer that function to return a 
> marker instead.)

Good question.  You mean that it would have been better to return a 
Lisp_Object, right?  I don't recall exactly, I think it was because in the 
calls to SET_BUF_BEGV_BOTH/SET_BUF_ZV_BOTH (which are the only places 
where the return value of labeled_restrictions_get_bound are used) one can 
use the pointer to a struct Lisp_Marker immediately, whereas a call to 
XMARKER would have been necessary if a Lisp_Object had been used.

> >   record_unwind_protect (save_restriction_restore, save_restriction_save ());
> > + labeled_restrictions_remove_in_current_buffer ();
>
> Why are we removing the restrictions as part of write-region?

We are removing them temporarily, just before the Fwiden, and they are 
restored by save_restriction_restore.

> >           record_unwind_protect (save_restriction_restore,
> >                                  save_restriction_save ());
> > +         labeled_restrictions_remove_in_current_buffer ();
> >           Fwiden ();
>
> And why here?

For the same reason: calls to Fwiden which are preceded by a 
"record_unwind_protect (save_restriction_restore, save_restriction_save 
());" are meant to temporarily widen the buffer, and restore the 
restrictions upon returning from the function.  So we temporarily remove 
labeled restrictions as well (and they are restored by 
save_restriction_restore, too).

> >           record_unwind_protect (save_restriction_restore, 
> > save_restriction_save ());
> > +         labeled_restrictions_remove_in_current_buffer ();
>
> And here?

For the same reason again ;-)

> >    record_unwind_protect (save_restriction_restore, save_restriction_save ());
> > +  labeled_restrictions_remove_in_current_buffer ();
> >    Fwiden ();
> >    val = display_count_lines (start_byte, limit_byte, count, byte_pos_ptr);
>
> Why do we remove the restrictions here?

... and again ;-)

> > +   The corresponding function 'get_medium_narrowing_zv' (and
> > +   'medium_narrowing_zv' field in 'struct it') is not used to set the
> > +   end limit of a the restriction, which is again unnecessary, but to
>                   ^^^^^
> Typo.

Good catch, thanks!

> > +static ptrdiff_t
> > +get_nearby_bol_pos (ptrdiff_t pos)
> > +{
> > +  ptrdiff_t start, pos_bytepos, cur, next, found, bol = BEGV - 1;
> > +  int dist;
> > +  for (dist = 500; dist <= 500000; dist *= 10)
> > +    {
> > +      pos_bytepos = pos == BEGV ? BEGV_BYTE : CHAR_TO_BYTE (pos);
> > +      start = pos - dist < BEGV ? BEGV : pos - dist;
> > +      for (cur = start; cur < pos; cur = next)
> > +       {
> > +         next = find_newline1 (cur, CHAR_TO_BYTE (cur),
> > +                               pos, pos_bytepos,
> > +                               1, &found, NULL, false);
> > +         if (found)
> > +           bol = next;
> > +         else
> > +           break;
> > +       }
> > +      if (bol >= BEGV || start == BEGV)
> > +       return bol;
> > +      else
> > +       pos = pos - dist < BEGV ? BEGV : pos - dist;
> > +    }
> > +  return bol;
> > +}
>
> This function should have a commentary describing what it does.

Okay, I'll add that.

> Is it okay for this function to return a position > POS, its input?

Unless I misunderstood something, it cannot, because find_newline1 is 
called with end = pos and end_byte = pos_bytepos.