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

[Gregory Heytings <gregory@heytings.org>]

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.