Re: [RFC]: replace-region-contents

[Tassilo Horn]

Eli Zaretskii <address@hidden> writes:

>> if nobody complains, I'd like to install the following patch.
>
> No complaints from me, just a few comments.

Too late, but I'll happily do some more changes.

>> I've set too_expensive to 64 because then my benchmark was almost as
>> fast as the original version without `replace-buffer-contents', and I
>> couldn't find any difference in observable behavior in that value
>> anyway except lower values give much better performance but a too low
>> value like 10 will segfault.  My main concern was that point was at
>> the same position before and after pretty-printing my JSON.
>
> I'd prefer to expose this to Lisp, not hardcode the value.  We could
> use the hardcoded value in json.el, but I'd like to leave this to the
> application in the other cases.  I'm not sure such a small value will
> always be the right tradeoff, so I think we shouldn't decide just yet.
>
> If you agree with that, let's expose this via a new optional argument
> to replace-buffer-contents, and let's treat the value of nil as a very
> large integer value, i.e. "no limit".

Yes, fine with me.  I'll do that tomorrow.

>>  #undef ELEMENT
>>  #undef EQUAL
>> +#define USE_HEURISTIC
>> +
>> +#ifdef USE_HEURISTIC
>> +#define DIFFSEQ_HEURISTIC
>> +#endif
>>  
>>  /* Counter used to rarely_quit in replace-buffer-contents.  */
>>  static unsigned short rbc_quitcounter;
>> @@ -2017,8 +2022,11 @@ differences between the two buffers.  */)
>>      .insertions = SAFE_ALLOCA (ins_bytes),
>>      .fdiag = buffer + size_b + 1,
>>      .bdiag = buffer + diags + size_b + 1,
>> +#ifdef DIFFSEQ_HEURISTIC
>> +    .heuristic = true,
>> +#endif
>
> Why do we need this triple-level conditional?  If we think the
> heuristic is a good idea, let's just enable it unconditionally.  If
> someone wants to modify Emacs on the C level, they can edit the source
> as easily as they can invoke the compiler with a -U switch.

Ok.

> Finally, I think this needs NEWS entries, both regarding the new
> function and the additional argument to replace-buffer-contents.  The
> latter is also documented in the ELisp manual, which will need to be
> updated.

I'll write it.

Thanks,
Tassilo