* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'
@ 2019-06-13 13:48 Pip Cet
2019-06-13 16:36 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Pip Cet @ 2019-06-13 13:48 UTC (permalink / raw)
To: 36190
In emacs -Q, evaluating:
(let ((buffer1 (generate-new-buffer "A"))
(buffer2 (generate-new-buffer "B")))
(with-current-buffer buffer2
(insert "BBB"))
(with-current-buffer buffer1
(add-hook 'after-change-functions
(lambda (beg end len)
(message "%S %S %S"
beg end len))
nil t)
(put-text-property 1 4 'read-only t buffer2)))
results in a "1 4 3" message. I would have expected no message, as
buffer2 was modified and buffer1, whose after-change-functions I'd
set, wasn't.
I've looked at the code, and it appears no particular provisions are
being made to make sure we switch to the modified buffer before
calling signal_after_change().
As far as I can tell, this makes `put-text-property' with a buffer
argument pretty useless. Am I missing something? Is this expected
behavior somehow?
^ permalink raw reply [flat|nested] 22+ messages in thread* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 13:48 bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' Pip Cet @ 2019-06-13 16:36 ` Eli Zaretskii 2019-06-13 18:48 ` Pip Cet 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2019-06-13 16:36 UTC (permalink / raw) To: Pip Cet; +Cc: 36190 > From: Pip Cet <pipcet@gmail.com> > Date: Thu, 13 Jun 2019 13:48:40 +0000 > > I've looked at the code, and it appears no particular provisions are > being made to make sure we switch to the modified buffer before > calling signal_after_change(). > > As far as I can tell, this makes `put-text-property' with a buffer > argument pretty useless. Only if you have a buffer-local value of after-change-functions. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 16:36 ` Eli Zaretskii @ 2019-06-13 18:48 ` Pip Cet 2019-06-13 19:05 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Pip Cet @ 2019-06-13 18:48 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 [-- Attachment #1: Type: text/plain, Size: 1599 bytes --] On Thu, Jun 13, 2019 at 4:36 PM Eli Zaretskii <eliz@gnu.org> wrote: > > > From: Pip Cet <pipcet@gmail.com> > > Date: Thu, 13 Jun 2019 13:48:40 +0000 > > > > I've looked at the code, and it appears no particular provisions are > > being made to make sure we switch to the modified buffer before > > calling signal_after_change(). > > > > As far as I can tell, this makes `put-text-property' with a buffer > > argument pretty useless. > > Only if you have a buffer-local value of after-change-functions. I'm not sure what you're saying. I'm seeing weird behavior in these cases: - buffer-local value of after-change-functions - global value of after-change functions (current-buffer is wrong!) - overlay property modification-hooks (let ((buffer1 (generate-new-buffer "A")) (buffer2 (generate-new-buffer "B"))) (with-current-buffer buffer2 (insert "BBB")) (with-current-buffer buffer1 (insert "AAA") (overlay-put (make-overlay 1 4 buffer1) 'modification-hooks (list (lambda (&rest args) (message "%S" args)))) (goto-char 2) (insert "AAA") (put-text-property 1 4 'read-only t buffer2))) produces three calls to the lambda, but should produce only two. (The last_overlay_modification_hooks logic is a bit weird, thus the second insertion). That seems pretty wrong to me. In which cases do you think we're seeing the right behavior? Here's a first patch, which adds a "buffer" argument to signal_after_change, to be explicit about where the change happens. It should be pretty cheap in the case where we don't switch buffers. [-- Attachment #2: 0001-Switch-to-modified-buffer-in-signal_after_change.patch --] [-- Type: text/x-patch, Size: 17265 bytes --] From ff40a90cc1e4ade9848b6cb01ea9e415d4e82d8f Mon Sep 17 00:00:00 2001 From: Pip Cet <pipcet@gmail.com> Date: Thu, 13 Jun 2019 14:06:46 +0000 Subject: [PATCH] Switch to modified buffer in signal_after_change * src/insdel.c (signal_after_change): Add buffer argument. * src/casefiddle.c (casify_region), src/decompress.c (unwind_decompress, Fzlib_decompress_region), src/editfns.c (Freplace_buffer_contents, Ftranslate_region_internal, Ftranspose_regions), src/fileio.c (Finsert_file_contents), src/fns.c (Fbase64_decode_region), src/insdel.c (insert, insert_and_inherit, insert_before_markers, insert_before_markers_and_inherit, insert_from_string, insert_from_string_before_markers, insert_from_buffer, replace_range, del_range_1, del_range_byte, del_range_both, Fcombine_after_change_execute), src/json.c (Fjson_insert), src/print.c (PRINTFINISH): use current_buffer. * src/textprop.c (add_text_properties_1, set_text_properties, Fremove_text_properties, Fremove_list_of_text_properties): use correct buffer. --- src/casefiddle.c | 2 +- src/decompress.c | 4 ++-- src/editfns.c | 8 ++++---- src/fileio.c | 2 +- src/fns.c | 2 +- src/insdel.c | 30 +++++++++++++++++------------- src/json.c | 2 +- src/lisp.h | 2 +- src/print.c | 3 ++- src/textprop.c | 22 +++++++++++----------- 10 files changed, 41 insertions(+), 36 deletions(-) diff --git a/src/casefiddle.c b/src/casefiddle.c index 3f407eaded..d7e29d10ae 100644 --- a/src/casefiddle.c +++ b/src/casefiddle.c @@ -509,7 +509,7 @@ casify_region (enum case_action flag, Lisp_Object b, Lisp_Object e) if (start >= 0) { - signal_after_change (start, end - start - added, end - start); + signal_after_change (current_buffer, start, end - start - added, end - start); update_compositions (start, end, CHECK_ALL); } diff --git a/src/decompress.c b/src/decompress.c index 4ca6a50b2a..831cc78fe5 100644 --- a/src/decompress.c +++ b/src/decompress.c @@ -88,7 +88,7 @@ unwind_decompress (void *ddata) update_compositions (data->start, data->start, CHECK_HEAD); /* "Balance" the before-change-functions call, which would otherwise be left "hanging". */ - signal_after_change (data->orig, data->start - data->orig, + signal_after_change (current_buffer, data->orig, data->start - data->orig, data->start - data->orig); } /* Put point where it was, or if the buffer has shrunk because the @@ -227,7 +227,7 @@ DEFUN ("zlib-decompress-region", Fzlib_decompress_region, del_range_2 (istart, istart, /* byte and char offsets are the same. */ iend, iend, 0); - signal_after_change (istart, iend - istart, unwind_data.nbytes); + signal_after_change (current_buffer, istart, iend - istart, unwind_data.nbytes); update_compositions (istart, istart, CHECK_HEAD); return unbind_to (count, ret); diff --git a/src/editfns.c b/src/editfns.c index ee538e50e2..9ada23ff61 100644 --- a/src/editfns.c +++ b/src/editfns.c @@ -2148,7 +2148,7 @@ DEFUN ("replace-buffer-contents", Freplace_buffer_contents, if (modification_hooks_inhibited) { - signal_after_change (BEGV, size_a, ZV - BEGV); + signal_after_change (current_buffer, BEGV, size_a, ZV - BEGV); update_compositions (BEGV, ZV, CHECK_INSIDE); } @@ -2412,7 +2412,7 @@ #define COMBINING_BOTH (COMBINING_BEFORE | COMBINING_AFTER) if (changed > 0) { - signal_after_change (changed, + signal_after_change (current_buffer, changed, last_changed - changed, last_changed - changed); update_compositions (changed, last_changed, CHECK_ALL); } @@ -2600,7 +2600,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal, record_change (pos, 1); while (str_len-- > 0) *p++ = *str++; - signal_after_change (pos, 1, 1); + signal_after_change (current_buffer, pos, 1, 1); update_compositions (pos, pos + 1, CHECK_BORDER); } characters_changed++; @@ -4445,7 +4445,7 @@ DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5, adjust_markers_bytepos (start1, start1_byte, end2, end2_byte, 0); } - signal_after_change (start1, end2 - start1, end2 - start1); + signal_after_change (current_buffer, start1, end2 - start1, end2 - start1); return Qnil; } diff --git a/src/fileio.c b/src/fileio.c index b141f568d7..056da2cb9d 100644 --- a/src/fileio.c +++ b/src/fileio.c @@ -4555,7 +4555,7 @@ because (1) it preserves some marker positions and (2) it puts less data if (inserted > 0 && total > 0 && (NILP (visit) || !NILP (replace))) { - signal_after_change (PT, 0, inserted); + signal_after_change (current_buffer, PT, 0, inserted); update_compositions (PT, PT, CHECK_BORDER); } diff --git a/src/fns.c b/src/fns.c index eaa2c07fbe..4b0c271b46 100644 --- a/src/fns.c +++ b/src/fns.c @@ -3577,7 +3577,7 @@ DEFUN ("base64-decode-region", Fbase64_decode_region, Sbase64_decode_region, and delete the old. (Insert first in order to preserve markers.) */ TEMP_SET_PT_BOTH (XFIXNAT (beg), ibeg); insert_1_both (decoded, inserted_chars, decoded_length, 0, 1, 0); - signal_after_change (XFIXNAT (beg), 0, inserted_chars); + signal_after_change (current_buffer, XFIXNAT (beg), 0, inserted_chars); SAFE_FREE (); /* Delete the original text. */ diff --git a/src/insdel.c b/src/insdel.c index 85fffd8fd1..f9499a6ab3 100644 --- a/src/insdel.c +++ b/src/insdel.c @@ -669,7 +669,7 @@ insert (const char *string, ptrdiff_t nbytes) ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint; insert_1_both (string, len, nbytes, 0, 1, 0); opoint = PT - len; - signal_after_change (opoint, 0, len); + signal_after_change (current_buffer, opoint, 0, len); update_compositions (opoint, PT, CHECK_BORDER); } } @@ -684,7 +684,7 @@ insert_and_inherit (const char *string, ptrdiff_t nbytes) ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint; insert_1_both (string, len, nbytes, 1, 1, 0); opoint = PT - len; - signal_after_change (opoint, 0, len); + signal_after_change (current_buffer, opoint, 0, len); update_compositions (opoint, PT, CHECK_BORDER); } } @@ -729,7 +729,7 @@ insert_before_markers (const char *string, ptrdiff_t nbytes) ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint; insert_1_both (string, len, nbytes, 0, 1, 1); opoint = PT - len; - signal_after_change (opoint, 0, len); + signal_after_change (current_buffer, opoint, 0, len); update_compositions (opoint, PT, CHECK_BORDER); } } @@ -745,7 +745,7 @@ insert_before_markers_and_inherit (const char *string, ptrdiff_t len = chars_in_text ((unsigned char *) string, nbytes), opoint; insert_1_both (string, len, nbytes, 1, 1, 1); opoint = PT - len; - signal_after_change (opoint, 0, len); + signal_after_change (current_buffer, opoint, 0, len); update_compositions (opoint, PT, CHECK_BORDER); } } @@ -959,7 +959,7 @@ insert_from_string (Lisp_Object string, ptrdiff_t pos, ptrdiff_t pos_byte, insert_from_string_1 (string, pos, pos_byte, length, length_byte, inherit, 0); - signal_after_change (opoint, 0, PT - opoint); + signal_after_change (current_buffer, opoint, 0, PT - opoint); update_compositions (opoint, PT, CHECK_BORDER); } @@ -979,7 +979,7 @@ insert_from_string_before_markers (Lisp_Object string, insert_from_string_1 (string, pos, pos_byte, length, length_byte, inherit, 1); - signal_after_change (opoint, 0, PT - opoint); + signal_after_change (current_buffer, opoint, 0, PT - opoint); update_compositions (opoint, PT, CHECK_BORDER); } @@ -1135,7 +1135,7 @@ insert_from_buffer (struct buffer *buf, ptrdiff_t opoint = PT; insert_from_buffer_1 (buf, charpos, nchars, inherit); - signal_after_change (opoint, 0, PT - opoint); + signal_after_change (current_buffer, opoint, 0, PT - opoint); update_compositions (opoint, PT, CHECK_BORDER); } @@ -1530,7 +1530,7 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new, if (adjust_match_data) update_search_regs (from, to, from + SCHARS (new)); - signal_after_change (from, nchars_del, GPT - from); + signal_after_change (current_buffer, from, nchars_del, GPT - from); update_compositions (from, GPT, CHECK_BORDER); } \f @@ -1698,7 +1698,7 @@ del_range_1 (ptrdiff_t from, ptrdiff_t to, bool prepare, bool ret_string) to_byte = CHAR_TO_BYTE (to); deletion = del_range_2 (from, from_byte, to, to_byte, ret_string); - signal_after_change (from, to - from, 0); + signal_after_change (current_buffer, from, to - from, 0); update_compositions (from, from, CHECK_HEAD); return deletion; } @@ -1740,7 +1740,7 @@ del_range_byte (ptrdiff_t from_byte, ptrdiff_t to_byte) } del_range_2 (from, from_byte, to, to_byte, 0); - signal_after_change (from, to - from, 0); + signal_after_change (current_buffer, from, to - from, 0); update_compositions (from, from, CHECK_HEAD); } @@ -1784,7 +1784,7 @@ del_range_both (ptrdiff_t from, ptrdiff_t from_byte, } del_range_2 (from, from_byte, to, to_byte, 0); - signal_after_change (from, to - from, 0); + signal_after_change (current_buffer, from, to - from, 0); update_compositions (from, from, CHECK_HEAD); } @@ -2174,7 +2174,8 @@ signal_before_change (ptrdiff_t start_int, ptrdiff_t end_int, after the change. */ void -signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) +signal_after_change (struct buffer *buffer, + ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) { ptrdiff_t count = SPECPDL_INDEX (); struct rvoe_arg rvoe_arg; @@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) if (inhibit_modification_hooks) return; + record_unwind_current_buffer (); + set_buffer_internal (buffer); + /* If we are deferring calls to the after-change functions and there are no before-change functions, just record the args that we were going to use. */ @@ -2338,7 +2342,7 @@ DEFUN ("combine-after-change-execute", Fcombine_after_change_execute, Turn off the flag that defers them. */ record_unwind_protect (Fcombine_after_change_execute_1, Vcombine_after_change_calls); - signal_after_change (begpos, endpos - begpos - change, endpos - begpos); + signal_after_change (current_buffer, begpos, endpos - begpos - change, endpos - begpos); update_compositions (begpos, endpos, CHECK_ALL); return unbind_to (count, Qnil); diff --git a/src/json.c b/src/json.c index e2a4424463..ecb47f3ff4 100644 --- a/src/json.c +++ b/src/json.c @@ -789,7 +789,7 @@ DEFUN ("json-insert", Fjson_insert, Sjson_insert, 1, MANY, } /* Call after-change hooks. */ - signal_after_change (PT, 0, inserted); + signal_after_change (current_buffer, PT, 0, inserted); if (inserted > 0) { update_compositions (PT, PT, CHECK_BORDER); diff --git a/src/lisp.h b/src/lisp.h index 77fc22d118..a1ed4a3ce9 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -3685,7 +3685,7 @@ #define CONS_TO_INTEGER(cons, type, var) \ extern void prepare_to_modify_buffer (ptrdiff_t, ptrdiff_t, ptrdiff_t *); extern void prepare_to_modify_buffer_1 (ptrdiff_t, ptrdiff_t, ptrdiff_t *); extern void invalidate_buffer_caches (struct buffer *, ptrdiff_t, ptrdiff_t); -extern void signal_after_change (ptrdiff_t, ptrdiff_t, ptrdiff_t); +extern void signal_after_change (struct buffer *, ptrdiff_t, ptrdiff_t, ptrdiff_t); extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t); extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t, diff --git a/src/print.c b/src/print.c index 406abbf4a3..e94eaf36f9 100644 --- a/src/print.c +++ b/src/print.c @@ -178,7 +178,8 @@ #define PRINTFINISH \ else \ insert_1_both (print_buffer, print_buffer_pos, \ print_buffer_pos_byte, 0, 1, 0); \ - signal_after_change (PT - print_buffer_pos, 0, print_buffer_pos);\ + signal_after_change (current_buffer, PT - print_buffer_pos, \ + 0, print_buffer_pos); \ } \ if (free_print_buffer) \ { \ diff --git a/src/textprop.c b/src/textprop.c index ae42c44185..293ac02d9c 100644 --- a/src/textprop.c +++ b/src/textprop.c @@ -1215,7 +1215,7 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end, if (interval_has_all_properties (properties, i)) { if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); eassert (modified); @@ -1226,7 +1226,7 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end, { add_properties (properties, i, object, set_type); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1237,7 +1237,7 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end, copy_properties (unchanged, i); add_properties (properties, i, object, set_type); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1398,7 +1398,7 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties, set_text_properties_1 (start, end, properties, object, i); if (BUFFERP (object) && !NILP (coherent_change_p)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1565,7 +1565,7 @@ DEFUN ("remove-text-properties", Fremove_text_properties, { eassert (modified); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1574,7 +1574,7 @@ DEFUN ("remove-text-properties", Fremove_text_properties, { remove_properties (properties, Qnil, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1585,7 +1585,7 @@ DEFUN ("remove-text-properties", Fremove_text_properties, copy_properties (unchanged, i); remove_properties (properties, Qnil, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1663,7 +1663,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, if (modified) { if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; @@ -1677,7 +1677,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, modify_text_properties (object, start, end); remove_properties (Qnil, properties, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1690,7 +1690,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, modify_text_properties (object, start, end); remove_properties (Qnil, properties, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1709,7 +1709,7 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, if (modified) { if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), + signal_after_change (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start)); return Qt; -- 2.20.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 18:48 ` Pip Cet @ 2019-06-13 19:05 ` Eli Zaretskii 2019-06-13 19:27 ` Eli Zaretskii 2019-06-13 19:42 ` Pip Cet 0 siblings, 2 replies; 22+ messages in thread From: Eli Zaretskii @ 2019-06-13 19:05 UTC (permalink / raw) To: Pip Cet; +Cc: 36190 > From: Pip Cet <pipcet@gmail.com> > Date: Thu, 13 Jun 2019 18:48:23 +0000 > Cc: 36190@debbugs.gnu.org > > > > As far as I can tell, this makes `put-text-property' with a buffer > > > argument pretty useless. > > > > Only if you have a buffer-local value of after-change-functions. > > I'm not sure what you're saying. I'm saying that the buffer argument to put-text-property is pretty useless only if you consider after-change-functions. The primary purpose of put-text-property is to modify text properties, not to call after-change-functions. For that primary purpose, the buffer argument is not useless. > That seems pretty wrong to me. In which cases do you think we're > seeing the right behavior? Where did I say that this behavior was right? > Here's a first patch, which adds a "buffer" argument to > signal_after_change, to be explicit about where the change happens. It > should be pretty cheap in the case where we don't switch buffers. Not sure I have a clear idea of how you intend to use that additional argument. Are you suggesting that we switch to that buffer? If so, how is that different from not using the buffer argument at all, and instead wrapping the call to put-text-property with with-current-buffer? Also, passing current_buffer sounds redundant to me anyway, because in that case signal_after_change will not need to do anything that it doesn't already do. I would pass NULL instead. Thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 19:05 ` Eli Zaretskii @ 2019-06-13 19:27 ` Eli Zaretskii 2019-06-13 19:42 ` Pip Cet 1 sibling, 0 replies; 22+ messages in thread From: Eli Zaretskii @ 2019-06-13 19:27 UTC (permalink / raw) To: pipcet; +Cc: 36190 > Date: Thu, 13 Jun 2019 22:05:48 +0300 > From: Eli Zaretskii <eliz@gnu.org> > Cc: 36190@debbugs.gnu.org > > > Here's a first patch, which adds a "buffer" argument to > > signal_after_change, to be explicit about where the change happens. It > > should be pretty cheap in the case where we don't switch buffers. > > Not sure I have a clear idea of how you intend to use that additional > argument. Found it: > + record_unwind_current_buffer (); > + set_buffer_internal (buffer); ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 19:05 ` Eli Zaretskii 2019-06-13 19:27 ` Eli Zaretskii @ 2019-06-13 19:42 ` Pip Cet 2019-06-13 20:01 ` Eli Zaretskii 1 sibling, 1 reply; 22+ messages in thread From: Pip Cet @ 2019-06-13 19:42 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 On Thu, Jun 13, 2019 at 7:06 PM Eli Zaretskii <eliz@gnu.org> wrote: > > > From: Pip Cet <pipcet@gmail.com> > > Date: Thu, 13 Jun 2019 18:48:23 +0000 > > Cc: 36190@debbugs.gnu.org > > > > > > As far as I can tell, this makes `put-text-property' with a buffer > > > > argument pretty useless. > > > > > > Only if you have a buffer-local value of after-change-functions. > > > > I'm not sure what you're saying. > > I'm saying that the buffer argument to put-text-property is pretty > useless only if you consider after-change-functions. The primary > purpose of put-text-property is to modify text properties, not to call > after-change-functions. For that primary purpose, the buffer argument > is not useless. Thanks for clarifying. I suppose you could say that it's after-change-functions (local, global, plus overlay modification hooks) that have become useless, or will be called spuriously and with potentially nonsensical arguments. For example, this would break: (push (lambda (beg end len) (message "%S" (buffer-substring beg end))) after-change-functions) > > That seems pretty wrong to me. In which cases do you think we're > > seeing the right behavior? > > Where did I say that this behavior was right? You said "only if", so I assumed you were asserting the contrapositive. > > Here's a first patch, which adds a "buffer" argument to > > signal_after_change, to be explicit about where the change happens. It > > should be pretty cheap in the case where we don't switch buffers. > > Not sure I have a clear idea of how you intend to use that additional > argument. Are you suggesting that we switch to that buffer? Yes: @@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) if (inhibit_modification_hooks) return; + record_unwind_current_buffer (); + set_buffer_internal (buffer); + /* If we are deferring calls to the after-change functions and there are no before-change functions, just record the args that we were going to use. */ > If so, > how is that different from not using the buffer argument at all, and > instead wrapping the call to put-text-property with > with-current-buffer? I don't think they're usefully different, but put-text-property doesn't appear to check the buffer is still live. > Also, passing current_buffer sounds redundant to me anyway, because in > that case signal_after_change will not need to do anything that it > doesn't already do. I would pass NULL instead. May I ask why? I think passing current_buffer is the clearest signal we can send to someone reusing the code that they might have to change this if they're dealing with more than one buffer. As a practical matter, it's hard to change the text property functions to use NULL when passed a nil argument, so we'd have functions using current_buffer and others using NULL, and that seems needlessly inconsistent. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 19:42 ` Pip Cet @ 2019-06-13 20:01 ` Eli Zaretskii 2019-06-13 20:57 ` Pip Cet 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2019-06-13 20:01 UTC (permalink / raw) To: Pip Cet; +Cc: 36190 > From: Pip Cet <pipcet@gmail.com> > Date: Thu, 13 Jun 2019 19:42:29 +0000 > Cc: 36190@debbugs.gnu.org > > > Not sure I have a clear idea of how you intend to use that additional > > argument. Are you suggesting that we switch to that buffer? > > Yes: > > @@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos, > ptrdiff_t lendel, ptrdiff_t lenins) > if (inhibit_modification_hooks) > return; > > + record_unwind_current_buffer (); > + set_buffer_internal (buffer); Ugh! switching buffers just to run a hook! This will kill performance in some cases. We had something similar with JSON parsing a few months ago. I wish we had a better alternative. Maybe we should warn in the documentation that calling these functions with BUFFER being other than the current buffer might hurt performance when after-change-functions is non-nil. > > Also, passing current_buffer sounds redundant to me anyway, because in > > that case signal_after_change will not need to do anything that it > > doesn't already do. I would pass NULL instead. > > May I ask why? To make the code speak for itself. With passing current_buffer, you now rely on subroutines of set_buffer_internal two or 3 levels down to test whether we are already in that buffer and do nothing. Meanwhile, you wasted cycles on 2 or 3 function calls, and forced someone who reads the code to go down that rabbit hole if they want to understand what happens in that particular case. > I think passing current_buffer is the clearest signal we can send to > someone reusing the code that they might have to change this if > they're dealing with more than one buffer. Each function has commentary, where you can say that NULL means not to switch buffers because we are already there. That is a more clear signal, IME. > As a practical matter, it's hard to change the text property functions > to use NULL when passed a nil argument How is it harder than passing current_buffer? > so we'd have functions using current_buffer and others using NULL, > and that seems needlessly inconsistent. Sorry, I don't see any inconsistency. We do such things (for other kinds of arguments) all over the place. It's really a matter of stylistic preferences, but you did ask why... ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 20:01 ` Eli Zaretskii @ 2019-06-13 20:57 ` Pip Cet 2019-06-13 21:37 ` Pip Cet 2019-06-14 7:36 ` Eli Zaretskii 0 siblings, 2 replies; 22+ messages in thread From: Pip Cet @ 2019-06-13 20:57 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 On Thu, Jun 13, 2019 at 8:01 PM Eli Zaretskii <eliz@gnu.org> wrote: > > From: Pip Cet <pipcet@gmail.com> > > Date: Thu, 13 Jun 2019 19:42:29 +0000 > > Cc: 36190@debbugs.gnu.org > > > > > Not sure I have a clear idea of how you intend to use that additional > > > argument. Are you suggesting that we switch to that buffer? > > > > Yes: > > > > @@ -2183,6 +2184,9 @@ signal_after_change (ptrdiff_t charpos, > > ptrdiff_t lendel, ptrdiff_t lenins) > > if (inhibit_modification_hooks) > > return; > > > > + record_unwind_current_buffer (); > > + set_buffer_internal (buffer); > > Ugh! switching buffers just to run a hook! > This will kill performance > in some cases. I really don't think it will have a noticeable impact on performance, but if you can think of a scenario, we could try to fix it. > We had something similar with JSON parsing a few > months ago. > I wish we had a better alternative. (Such as not calling regular modification hooks for text property changes?) > Maybe we should warn > in the documentation that calling these functions with BUFFER being > other than the current buffer might hurt performance when > after-change-functions is non-nil. It'll hurt performance even when after-change-functions is nil, so such a warning would be overspecific. > > As a practical matter, it's hard to change the text property functions > > to use NULL when passed a nil argument > > How is it harder than passing current_buffer? The code path goes through if (NILP (object)) XSETBUFFER (object, current_buffer); > It's really a matter of stylistic preferences, but you did ask why... It was out of genuine interest, because passing NULL to implicitly specify a default argument is something that people advocate against, and that C in particular has a history of avoiding (stdout isn't NULL, and post-VAX NULL isn't an empty string, for example). Thank you for responding, and I'll change my patch accordingly. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 20:57 ` Pip Cet @ 2019-06-13 21:37 ` Pip Cet 2019-06-14 7:41 ` Eli Zaretskii 2019-06-14 7:36 ` Eli Zaretskii 1 sibling, 1 reply; 22+ messages in thread From: Pip Cet @ 2019-06-13 21:37 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 [-- Attachment #1: Type: text/plain, Size: 103 bytes --] Here's a simplified patch (I'm unsure about the right tab convention to use; I hope I got this right). [-- Attachment #2: 0001-Switch-to-correct-buffer-in-put-text-property-etc.patch --] [-- Type: text/x-patch, Size: 8227 bytes --] From 8664be1b0aa55731774fe2b010d8dd7259ca2d28 Mon Sep 17 00:00:00 2001 From: Pip Cet <pipcet@gmail.com> Date: Thu, 13 Jun 2019 21:21:50 +0000 Subject: [PATCH] Switch to correct buffer in put-text-property etc. * src/textprop.c (add_text_properties_1, set_text_properties) (Fremove_text_properties, Fremove_list_of_text_properties): use `signal_after_change_in_buffer'. * src/insdel.c (signal_after_change_in_buffer): New function. --- src/insdel.c | 19 +++++++++++++ src/lisp.h | 1 + src/textprop.c | 73 +++++++++++++++++++++++++++----------------------- 3 files changed, 60 insertions(+), 33 deletions(-) diff --git a/src/insdel.c b/src/insdel.c index 85fffd8fd1..0552044447 100644 --- a/src/insdel.c +++ b/src/insdel.c @@ -2253,6 +2253,25 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) unbind_to (count, Qnil); } +/* Signal a change immediately after it happens. + BUFFER is the buffer in which the change happened. + CHARPOS is the character position of the start of the changed text. + LENDEL is the number of characters of the text before the change. + (Not the whole buffer; just the part that was changed.) + LENINS is the number of characters in that part of the text + after the change. */ + +void +signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) +{ + ptrdiff_t count = SPECPDL_INDEX (); + + record_unwind_current_buffer (); + set_buffer_internal (buffer); + signal_after_change (charpos, lendel, lenins); + unbind_to (count, Qnil); +} + static void Fcombine_after_change_execute_1 (Lisp_Object val) { diff --git a/src/lisp.h b/src/lisp.h index 77fc22d118..4e05c9555b 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -3686,6 +3686,7 @@ #define CONS_TO_INTEGER(cons, type, var) \ extern void prepare_to_modify_buffer_1 (ptrdiff_t, ptrdiff_t, ptrdiff_t *); extern void invalidate_buffer_caches (struct buffer *, ptrdiff_t, ptrdiff_t); extern void signal_after_change (ptrdiff_t, ptrdiff_t, ptrdiff_t); +extern void signal_after_change_in_buffer (struct buffer *, ptrdiff_t, ptrdiff_t, ptrdiff_t); extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t); extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t, diff --git a/src/textprop.c b/src/textprop.c index ae42c44185..1781440b10 100644 --- a/src/textprop.c +++ b/src/textprop.c @@ -1215,8 +1215,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end, if (interval_has_all_properties (properties, i)) { if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); eassert (modified); return Qt; @@ -1226,8 +1226,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end, { add_properties (properties, i, object, set_type); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1237,8 +1237,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end, copy_properties (unchanged, i); add_properties (properties, i, object, set_type); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1398,8 +1398,9 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties, set_text_properties_1 (start, end, properties, object, i); if (BUFFERP (object) && !NILP (coherent_change_p)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1565,8 +1566,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties, { eassert (modified); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1574,8 +1576,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties, { remove_properties (properties, Qnil, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1585,8 +1588,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties, copy_properties (unchanged, i); remove_properties (properties, Qnil, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1663,9 +1667,10 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, if (modified) { if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } else @@ -1677,8 +1682,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, modify_text_properties (object, start, end); remove_properties (Qnil, properties, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } else @@ -1690,8 +1696,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, modify_text_properties (object, start, end); remove_properties (Qnil, properties, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } } @@ -1705,18 +1712,18 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, len -= LENGTH (i); i = next_interval (i); if (!i) - { - if (modified) - { - if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); - return Qt; - } - else - return Qnil; - } + { + if (modified) + { + if (BUFFERP (object)) + signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); + return Qt; + } + else + return Qnil; + } } } \f -- 2.20.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 21:37 ` Pip Cet @ 2019-06-14 7:41 ` Eli Zaretskii 2019-06-14 11:14 ` Pip Cet 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2019-06-14 7:41 UTC (permalink / raw) To: Pip Cet; +Cc: 36190 > From: Pip Cet <pipcet@gmail.com> > Date: Thu, 13 Jun 2019 21:37:40 +0000 > Cc: 36190@debbugs.gnu.org > > +/* Signal a change immediately after it happens. > + BUFFER is the buffer in which the change happened. > + CHARPOS is the character position of the start of the changed text. > + LENDEL is the number of characters of the text before the change. > + (Not the whole buffer; just the part that was changed.) > + LENINS is the number of characters in that part of the text > + after the change. */ I would just say "Like signal_after_change, but ..." and describe only the BUFFER argument. > +void > +signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) > +{ > + ptrdiff_t count = SPECPDL_INDEX (); > + > + record_unwind_current_buffer (); > + set_buffer_internal (buffer); > + signal_after_change (charpos, lendel, lenins); > + unbind_to (count, Qnil); > +} I still think we should explicitly detect the current_buffer case here and if so, avoid the calls to everything else except signal_after_change itself. But I indeed like this patch better, although the concerns over the performance hit are still present. Thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-14 7:41 ` Eli Zaretskii @ 2019-06-14 11:14 ` Pip Cet 2019-06-14 12:10 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Pip Cet @ 2019-06-14 11:14 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 [-- Attachment #1: Type: text/plain, Size: 1588 bytes --] On Fri, Jun 14, 2019 at 7:41 AM Eli Zaretskii <eliz@gnu.org> wrote: > > > From: Pip Cet <pipcet@gmail.com> > > Date: Thu, 13 Jun 2019 21:37:40 +0000 > > Cc: 36190@debbugs.gnu.org > > > > +/* Signal a change immediately after it happens. > > + BUFFER is the buffer in which the change happened. > > + CHARPOS is the character position of the start of the changed text. > > + LENDEL is the number of characters of the text before the change. > > + (Not the whole buffer; just the part that was changed.) > > + LENINS is the number of characters in that part of the text > > + after the change. */ > > I would just say "Like signal_after_change, but ..." and describe only > the BUFFER argument. > > > +void > > +signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) > > +{ > > + ptrdiff_t count = SPECPDL_INDEX (); > > + > > + record_unwind_current_buffer (); > > + set_buffer_internal (buffer); > > + signal_after_change (charpos, lendel, lenins); > > + unbind_to (count, Qnil); > > +} > > I still think we should explicitly detect the current_buffer case here > and if so, avoid the calls to everything else except > signal_after_change itself. > > But I indeed like this patch better, although the concerns over the > performance hit are still present. Thanks. Okay, I fixed those two issues. As for the performance problem, should we amend the documentation to state that if many changes are made, it's better to use `with-current-buffer' instead of repeatedly calling put-text-property with a buffer argument? [-- Attachment #2: 0001-Switch-to-correct-buffer-in-put-text-property-etc.patch --] [-- Type: text/x-patch, Size: 7946 bytes --] From b23ebe451664b0f50a02fb2e4ee5a41877dfe32d Mon Sep 17 00:00:00 2001 From: Pip Cet <pipcet@gmail.com> Date: Fri, 14 Jun 2019 11:05:31 +0000 Subject: [PATCH] Switch to correct buffer in put-text-property etc. * src/textprop.c (add_text_properties_1, set_text_properties) (Fremove_text_properties, Fremove_list_of_text_properties): use `signal_after_change_in_buffer'. * src/insdel.c (signal_after_change_in_buffer): New function. --- src/insdel.c | 16 +++++++++++ src/lisp.h | 1 + src/textprop.c | 73 +++++++++++++++++++++++++++----------------------- 3 files changed, 57 insertions(+), 33 deletions(-) diff --git a/src/insdel.c b/src/insdel.c index 85fffd8fd1..49aca94e03 100644 --- a/src/insdel.c +++ b/src/insdel.c @@ -2253,6 +2253,22 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) unbind_to (count, Qnil); } +/* Like `signal_after_change', but switch to BUFFER first. */ + +void +signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins) +{ + ptrdiff_t count = SPECPDL_INDEX (); + + if (buffer != current_buffer) + { + record_unwind_current_buffer (); + set_buffer_internal (buffer); + } + signal_after_change (charpos, lendel, lenins); + unbind_to (count, Qnil); +} + static void Fcombine_after_change_execute_1 (Lisp_Object val) { diff --git a/src/lisp.h b/src/lisp.h index 77fc22d118..4e05c9555b 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -3686,6 +3686,7 @@ #define CONS_TO_INTEGER(cons, type, var) \ extern void prepare_to_modify_buffer_1 (ptrdiff_t, ptrdiff_t, ptrdiff_t *); extern void invalidate_buffer_caches (struct buffer *, ptrdiff_t, ptrdiff_t); extern void signal_after_change (ptrdiff_t, ptrdiff_t, ptrdiff_t); +extern void signal_after_change_in_buffer (struct buffer *, ptrdiff_t, ptrdiff_t, ptrdiff_t); extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t); extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t, diff --git a/src/textprop.c b/src/textprop.c index ae42c44185..1781440b10 100644 --- a/src/textprop.c +++ b/src/textprop.c @@ -1215,8 +1215,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end, if (interval_has_all_properties (properties, i)) { if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); eassert (modified); return Qt; @@ -1226,8 +1226,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end, { add_properties (properties, i, object, set_type); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1237,8 +1237,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end, copy_properties (unchanged, i); add_properties (properties, i, object, set_type); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1398,8 +1398,9 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties, set_text_properties_1 (start, end, properties, object, i); if (BUFFERP (object) && !NILP (coherent_change_p)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1565,8 +1566,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties, { eassert (modified); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1574,8 +1576,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties, { remove_properties (properties, Qnil, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1585,8 +1588,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties, copy_properties (unchanged, i); remove_properties (properties, Qnil, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } @@ -1663,9 +1667,10 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, if (modified) { if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } else @@ -1677,8 +1682,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, modify_text_properties (object, start, end); remove_properties (Qnil, properties, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } else @@ -1690,8 +1696,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, modify_text_properties (object, start, end); remove_properties (Qnil, properties, i, object); if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); + signal_after_change_in_buffer (XBUFFER (object), + XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); return Qt; } } @@ -1705,18 +1712,18 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, len -= LENGTH (i); i = next_interval (i); if (!i) - { - if (modified) - { - if (BUFFERP (object)) - signal_after_change (XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start), - XFIXNUM (end) - XFIXNUM (start)); - return Qt; - } - else - return Qnil; - } + { + if (modified) + { + if (BUFFERP (object)) + signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start), + XFIXNUM (end) - XFIXNUM (start)); + return Qt; + } + else + return Qnil; + } } } \f -- 2.20.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-14 11:14 ` Pip Cet @ 2019-06-14 12:10 ` Eli Zaretskii 2019-06-15 15:14 ` Pip Cet 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2019-06-14 12:10 UTC (permalink / raw) To: Pip Cet; +Cc: 36190 > From: Pip Cet <pipcet@gmail.com> > Date: Fri, 14 Jun 2019 11:14:53 +0000 > Cc: 36190@debbugs.gnu.org > > Okay, I fixed those two issues. Thanks. > As for the performance problem, should we amend the documentation to > state that if many changes are made, it's better to use > `with-current-buffer' instead of repeatedly calling > put-text-property with a buffer argument? I think so, yes. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-14 12:10 ` Eli Zaretskii @ 2019-06-15 15:14 ` Pip Cet 2019-06-15 15:23 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Pip Cet @ 2019-06-15 15:14 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 [-- Attachment #1: Type: text/plain, Size: 497 bytes --] On Fri, Jun 14, 2019 at 12:11 PM Eli Zaretskii <eliz@gnu.org> wrote: > > From: Pip Cet <pipcet@gmail.com> > > Date: Fri, 14 Jun 2019 11:14:53 +0000 > > Cc: 36190@debbugs.gnu.org > > > > Okay, I fixed those two issues. > > Thanks. Except that calls set_buffer_internal twice for a single call to put-text-property. That's too slow, right? How about simply emulating (with-current-buffer buffer (put-text-property ...)), as the attached patch does? That should fix the immediate problem. Thanks! [-- Attachment #2: 0001-Update-current-buffer-when-changing-text-properties.patch --] [-- Type: text/x-patch, Size: 3781 bytes --] From c822e562aaa51af2fc6ffc7708124fec6bb333e4 Mon Sep 17 00:00:00 2001 From: Pip Cet <pipcet@gmail.com> Date: Sat, 15 Jun 2019 15:12:14 +0000 Subject: [PATCH] Update current buffer when changing text properties. * src/textprop.c (add_text_properties_1, set_text_properties) (set_text_properties_1, Fremove_text_properties): --- src/textprop.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/textprop.c b/src/textprop.c index ae42c44185..46de372b61 100644 --- a/src/textprop.c +++ b/src/textprop.c @@ -1141,6 +1141,15 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change, add_text_properties_1 (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object, enum property_set_type set_type) { + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, add_text_properties_1 (start, end, properties, + object, set_type)); + } + INTERVAL i, unchanged; ptrdiff_t s, len; bool modified = false; @@ -1342,6 +1351,15 @@ face(s) are retained. This is done by setting the `face' property to set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object, Lisp_Object coherent_change_p) { + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, set_text_properties (start, end, properties, + object, coherent_change_p)); + } + INTERVAL i; bool first_time = true; @@ -1412,6 +1430,17 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties, set_text_properties_1 (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object, INTERVAL i) { + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + + set_text_properties_1 (start, end, properties, object, i); + unbind_to (count, Qnil); + return; + } + INTERVAL prev_changed = NULL; ptrdiff_t s = XFIXNUM (start); ptrdiff_t len = XFIXNUM (end) - s; @@ -1494,6 +1523,15 @@ DEFUN ("remove-text-properties", Fremove_text_properties, Use `set-text-properties' if you want to remove all text properties. */) (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object) { + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, Fremove_text_properties (start, end, properties, + object)); + } + INTERVAL i, unchanged; ptrdiff_t s, len; bool modified = false; @@ -1606,6 +1644,16 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, Return t if any property was actually removed, nil otherwise. */) (Lisp_Object start, Lisp_Object end, Lisp_Object list_of_properties, Lisp_Object object) { + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, Fremove_list_of_text_properties (start, end, + list_of_properties, + object)); + } + INTERVAL i, unchanged; ptrdiff_t s, len; bool modified = false; -- 2.20.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-15 15:14 ` Pip Cet @ 2019-06-15 15:23 ` Eli Zaretskii 2019-06-15 19:27 ` Pip Cet 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2019-06-15 15:23 UTC (permalink / raw) To: Pip Cet; +Cc: 36190 > From: Pip Cet <pipcet@gmail.com> > Date: Sat, 15 Jun 2019 15:14:43 +0000 > Cc: 36190@debbugs.gnu.org > > Except that calls set_buffer_internal twice for a single call to > put-text-property. That's too slow, right? > > How about simply emulating (with-current-buffer buffer > (put-text-property ...)), as the attached patch does? That should fix > the immediate problem. Looks better to me, but I'd suggest a comment explaining why we do such an unusual thing. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-15 15:23 ` Eli Zaretskii @ 2019-06-15 19:27 ` Pip Cet 2019-07-06 8:08 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Pip Cet @ 2019-06-15 19:27 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 [-- Attachment #1: Type: text/plain, Size: 724 bytes --] On Sat, Jun 15, 2019 at 3:23 PM Eli Zaretskii <eliz@gnu.org> wrote: > > From: Pip Cet <pipcet@gmail.com> > > Date: Sat, 15 Jun 2019 15:14:43 +0000 > > Cc: 36190@debbugs.gnu.org > > > > How about simply emulating (with-current-buffer buffer > > (put-text-property ...)), as the attached patch does? That should fix > > the immediate problem. > > Looks better to me, but I'd suggest a comment explaining why we do > such an unusual thing. Sounds good to me. The attached patch contains the comments and a hint in the documentation. (Maybe the comments should be a FIXME, because there's really no need to switch buffers at all when there are no applicable hooks?) Obviously, feel free to reword as you consider appropriate. [-- Attachment #2: 0001-Update-current-buffer-when-changing-text-properties.patch --] [-- Type: text/x-patch, Size: 4965 bytes --] From 01784e6b16c40aef362f39519a4e8b29982c8fcd Mon Sep 17 00:00:00 2001 From: Pip Cet <pipcet@gmail.com> Date: Sat, 15 Jun 2019 15:12:14 +0000 Subject: [PATCH] Update current buffer when changing text properties. * src/textprop.c (add_text_properties_1, set_text_properties) (set_text_properties_1, Fremove_text_properties): --- doc/lispref/text.texi | 2 +- src/textprop.c | 58 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/doc/lispref/text.texi b/doc/lispref/text.texi index 2e7c497f57..035fea6a6b 100644 --- a/doc/lispref/text.texi +++ b/doc/lispref/text.texi @@ -2800,7 +2800,7 @@ Examining Properties These functions handle both strings and buffers. Keep in mind that positions in a string start from 0, whereas positions in a buffer start -from 1. +from 1. Passing a buffer other than the current buffer may be slow. @defun get-text-property pos prop &optional object This function returns the value of the @var{prop} property of the diff --git a/src/textprop.c b/src/textprop.c index ae42c44185..56f276b9ad 100644 --- a/src/textprop.c +++ b/src/textprop.c @@ -1141,6 +1141,17 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change, add_text_properties_1 (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object, enum property_set_type set_type) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, add_text_properties_1 (start, end, properties, + object, set_type)); + } + INTERVAL i, unchanged; ptrdiff_t s, len; bool modified = false; @@ -1342,6 +1353,17 @@ face(s) are retained. This is done by setting the `face' property to set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object, Lisp_Object coherent_change_p) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, set_text_properties (start, end, properties, + object, coherent_change_p)); + } + INTERVAL i; bool first_time = true; @@ -1412,6 +1434,19 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties, set_text_properties_1 (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object, INTERVAL i) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + + set_text_properties_1 (start, end, properties, object, i); + unbind_to (count, Qnil); + return; + } + INTERVAL prev_changed = NULL; ptrdiff_t s = XFIXNUM (start); ptrdiff_t len = XFIXNUM (end) - s; @@ -1494,6 +1529,17 @@ DEFUN ("remove-text-properties", Fremove_text_properties, Use `set-text-properties' if you want to remove all text properties. */) (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, Fremove_text_properties (start, end, properties, + object)); + } + INTERVAL i, unchanged; ptrdiff_t s, len; bool modified = false; @@ -1606,6 +1652,18 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, Return t if any property was actually removed, nil otherwise. */) (Lisp_Object start, Lisp_Object end, Lisp_Object list_of_properties, Lisp_Object object) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, Fremove_list_of_text_properties (start, end, + list_of_properties, + object)); + } + INTERVAL i, unchanged; ptrdiff_t s, len; bool modified = false; -- 2.20.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-15 19:27 ` Pip Cet @ 2019-07-06 8:08 ` Eli Zaretskii 2019-07-06 15:27 ` Pip Cet 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2019-07-06 8:08 UTC (permalink / raw) To: Pip Cet; +Cc: 36190 > From: Pip Cet <pipcet@gmail.com> > Date: Sat, 15 Jun 2019 19:27:30 +0000 > Cc: 36190@debbugs.gnu.org > > > Looks better to me, but I'd suggest a comment explaining why we do > > such an unusual thing. > > Sounds good to me. The attached patch contains the comments and a hint > in the documentation. (Maybe the comments should be a FIXME, because > there's really no need to switch buffers at all when there are no > applicable hooks?) I'm okay with adding such a FIXME. > Obviously, feel free to reword as you consider appropriate. LGTM, with a couple of minor comments below. Feel free to push after fixing those. > + /* Ensure we run the modification hooks for the right buffer, > + without switching buffers twice (bug 36190). */ ^^ Here and elsewhere please keep 2 spaces between the period and "*/" > + if (BUFFERP (object) && XBUFFER (object) != current_buffer) > + { > + ptrdiff_t count = SPECPDL_INDEX (); > + record_unwind_current_buffer (); > + set_buffer_internal (XBUFFER (object)); > + return unbind_to (count, Fremove_list_of_text_properties (start, end, > + list_of_properties, > + object)); > + } The indentation here is too deep; suggest reformatting, e.g., by inserting a newline before Fremove_list_of_text_properties, and then reindenting the rest. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-07-06 8:08 ` Eli Zaretskii @ 2019-07-06 15:27 ` Pip Cet 2019-07-06 16:22 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Pip Cet @ 2019-07-06 15:27 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 [-- Attachment #1: Type: text/plain, Size: 612 bytes --] On Sat, Jul 6, 2019 at 8:08 AM Eli Zaretskii <eliz@gnu.org> wrote: > > Sounds good to me. The attached patch contains the comments and a hint > > in the documentation. (Maybe the comments should be a FIXME, because > > there's really no need to switch buffers at all when there are no > > applicable hooks?) > > I'm okay with adding such a FIXME. Added. > > > Obviously, feel free to reword as you consider appropriate. > > LGTM, with a couple of minor comments below. Feel free to push after > fixing those. I don't have push access, so I hope it's okay just to send the new patch. Thanks for the review! [-- Attachment #2: 0001-Update-current-buffer-when-changing-text-properties.patch --] [-- Type: text/x-patch, Size: 5401 bytes --] From 7e3b17ce5704b5327b977673382688f8e7f5b4c9 Mon Sep 17 00:00:00 2001 From: Pip Cet <pipcet@gmail.com> Date: Sat, 6 Jul 2019 15:21:04 +0000 Subject: [PATCH] Update current buffer when changing text properties. * src/textprop.c (add_text_properties_1, set_text_properties) (set_text_properties_1, Fremove_text_properties): Switch buffer if necessary. * doc/lispref/text.texi (Examining Properties): Document performance FIXME. --- doc/lispref/text.texi | 2 +- src/textprop.c | 66 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/doc/lispref/text.texi b/doc/lispref/text.texi index c4fc5247a1..ca0dd6642d 100644 --- a/doc/lispref/text.texi +++ b/doc/lispref/text.texi @@ -2800,7 +2800,7 @@ Examining Properties These functions handle both strings and buffers. Keep in mind that positions in a string start from 0, whereas positions in a buffer start -from 1. +from 1. Passing a buffer other than the current buffer may be slow. @defun get-text-property pos prop &optional object This function returns the value of the @var{prop} property of the diff --git a/src/textprop.c b/src/textprop.c index 9023f4efa0..44c333256a 100644 --- a/src/textprop.c +++ b/src/textprop.c @@ -1142,6 +1142,18 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change, add_text_properties_1 (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object, enum property_set_type set_type) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). FIXME: Switching + buffers is slow and often unnecessary. */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, add_text_properties_1 (start, end, properties, + object, set_type)); + } + INTERVAL i, unchanged; ptrdiff_t s, len; bool modified = false; @@ -1343,6 +1355,19 @@ face(s) are retained. This is done by setting the `face' property to set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object, Lisp_Object coherent_change_p) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). FIXME: Switching + buffers is slow and often unnecessary. */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, + set_text_properties (start, end, properties, + object, coherent_change_p)); + } + INTERVAL i; bool first_time = true; @@ -1413,6 +1438,20 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties, set_text_properties_1 (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object, INTERVAL i) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). FIXME: Switching + buffers is slow and often unnecessary. */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + + set_text_properties_1 (start, end, properties, object, i); + unbind_to (count, Qnil); + return; + } + INTERVAL prev_changed = NULL; ptrdiff_t s = XFIXNUM (start); ptrdiff_t len = XFIXNUM (end) - s; @@ -1495,6 +1534,19 @@ DEFUN ("remove-text-properties", Fremove_text_properties, Use `set-text-properties' if you want to remove all text properties. */) (Lisp_Object start, Lisp_Object end, Lisp_Object properties, Lisp_Object object) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). FIXME: Switching + buffers is slow and often unnecessary. */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, + Fremove_text_properties (start, end, properties, + object)); + } + INTERVAL i, unchanged; ptrdiff_t s, len; bool modified = false; @@ -1607,6 +1659,20 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties, Return t if any property was actually removed, nil otherwise. */) (Lisp_Object start, Lisp_Object end, Lisp_Object list_of_properties, Lisp_Object object) { + /* Ensure we run the modification hooks for the right buffer, + without switching buffers twice (bug 36190). FIXME: Switching + buffers is slow and often unnecessary. */ + if (BUFFERP (object) && XBUFFER (object) != current_buffer) + { + ptrdiff_t count = SPECPDL_INDEX (); + record_unwind_current_buffer (); + set_buffer_internal (XBUFFER (object)); + return unbind_to (count, + Fremove_list_of_text_properties (start, end, + list_of_properties, + object)); + } + INTERVAL i, unchanged; ptrdiff_t s, len; bool modified = false; -- 2.20.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-07-06 15:27 ` Pip Cet @ 2019-07-06 16:22 ` Eli Zaretskii 0 siblings, 0 replies; 22+ messages in thread From: Eli Zaretskii @ 2019-07-06 16:22 UTC (permalink / raw) To: Pip Cet; +Cc: 36190-done > From: Pip Cet <pipcet@gmail.com> > Date: Sat, 6 Jul 2019 15:27:41 +0000 > Cc: 36190@debbugs.gnu.org > > > LGTM, with a couple of minor comments below. Feel free to push after > > fixing those. > > I don't have push access, so I hope it's okay just to send the new patch. Thanks, pushed. Please in the future mention the bug number in the log message, and be sure NOT to end with a period the header line of the log message, per CONTRIBUTE. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-13 20:57 ` Pip Cet 2019-06-13 21:37 ` Pip Cet @ 2019-06-14 7:36 ` Eli Zaretskii 2019-06-17 11:38 ` Pip Cet 1 sibling, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2019-06-14 7:36 UTC (permalink / raw) To: Pip Cet; +Cc: 36190 > From: Pip Cet <pipcet@gmail.com> > Date: Thu, 13 Jun 2019 20:57:08 +0000 > Cc: 36190@debbugs.gnu.org > > > > + record_unwind_current_buffer (); > > > + set_buffer_internal (buffer); > > > > Ugh! switching buffers just to run a hook! This will kill > > performance in some cases. > > I really don't think it will have a noticeable impact on performance, > but if you can think of a scenario, we could try to fix it. Switching buffers means rebinding values of all the buffer-local variables, of which there could be quite a few. Or am I missing something? One scenario where this could be painful could be reading a stream of data that results in many changes in text properties, such as fontifying a buffer of program source by using syntactical analysis data received from a language server. If you read and apply the input one object at a time, this will result in many buffer switches. > > I wish we had a better alternative. > > (Such as not calling regular modification hooks for text property changes?) I thought about that, but I don't think this would be acceptable. > > Maybe we should warn > > in the documentation that calling these functions with BUFFER being > > other than the current buffer might hurt performance when > > after-change-functions is non-nil. > > It'll hurt performance even when after-change-functions is nil, so > such a warning would be overspecific. We could avoid switching buffers if the hook is nil, at least in principle. If not, it's even worse than I feared. > > > As a practical matter, it's hard to change the text property functions > > > to use NULL when passed a nil argument > > > > How is it harder than passing current_buffer? > > The code path goes through > > if (NILP (object)) > XSETBUFFER (object, current_buffer); I meant in the cases where you pass the literal current_buffer. But even the above is not a problem: struct buffer *b; if (NILP (object)) { XSETBUFFER (object, current_buffer); b = NULL; } else if (BUFFERP (object)) b = XBUFFER (object); [...] signal_after_change (b, ...); > It was out of genuine interest, because passing NULL to implicitly > specify a default argument is something that people advocate against, Not to specify the default, but to indicate that no action is needed at all wrt the buffer. It is similar to the last argument to 'strtol', for example. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-14 7:36 ` Eli Zaretskii @ 2019-06-17 11:38 ` Pip Cet 2019-06-17 15:59 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: Pip Cet @ 2019-06-17 11:38 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 On Fri, Jun 14, 2019 at 7:36 AM Eli Zaretskii <eliz@gnu.org> wrote: > > From: Pip Cet <pipcet@gmail.com> > > Date: Thu, 13 Jun 2019 20:57:08 +0000 > > Cc: 36190@debbugs.gnu.org > > > > > > + record_unwind_current_buffer (); > > > > + set_buffer_internal (buffer); > > > > > > Ugh! switching buffers just to run a hook! This will kill > > > performance in some cases. > > > > I really don't think it will have a noticeable impact on performance, > > but if you can think of a scenario, we could try to fix it. > > Switching buffers means rebinding values of all the buffer-local > variables, of which there could be quite a few. Or am I missing > something? I just don't see how the requirement to switch buffers for modifying text properties is so different, performance-wise, from the case of modifying buffer text; in the latter case, we simply accept we can do so only for the current buffer. In any case, the current code already switches buffers, it's just a question of doing so twice rather than once. > One scenario where this could be painful could be reading a stream of > data that results in many changes in text properties, such as > fontifying a buffer of program source by using syntactical analysis > data received from a language server. If you read and apply the input > one object at a time, this will result in many buffer switches. Yes, I agree. However, half of those buffer switches are probably because the language server output would be directed into a buffer; you could avoid those using a filter function, I suppose. > > > I wish we had a better alternative. > > > > (Such as not calling regular modification hooks for text property changes?) > > I thought about that, but I don't think this would be acceptable. It's certainly not something to be done on the spur of the moment, but it is something I feel Emacs did wrongly, perhaps because XEmacs did things differently, if I understand correctly. I'm not sure I'm aware of even a single place where text properties are used for something that's integrally part of buffer text. > > > Maybe we should warn > > > in the documentation that calling these functions with BUFFER being > > > other than the current buffer might hurt performance when > > > after-change-functions is non-nil. > > > > It'll hurt performance even when after-change-functions is nil, so > > such a warning would be overspecific. > > We could avoid switching buffers if the hook is nil, at least in > principle. If not, it's even worse than I feared. We could. I've looked at the code and I think the right thing to do, when someone has time to test things properly, is to rewrite all buffer-modifying functions to look like this: Lisp_Object hooks = run_before_change_hooks (...); modify_buffer (); run_after_change_hooks (hooks, ...); where run_before_change_hooks runs the before-change hooks but collects the modification hooks to be run after the modification in the same iteration. Right now, we're using global variables to achieve something similar, but, among other problems, that means modification hooks aren't reentrant. Modifying buffer B from buffer A's modification hooks sounds like it should be safe to me even when B has modification hooks, but it isn't. (In fact, I don't see why inhibit-modification-hooks isn't buffer-local). > > > > As a practical matter, it's hard to change the text property functions > > > > to use NULL when passed a nil argument > > > > > > How is it harder than passing current_buffer? > > > > The code path goes through > > > > if (NILP (object)) > > XSETBUFFER (object, current_buffer); > > I meant in the cases where you pass the literal current_buffer. > > But even the above is not a problem: > > struct buffer *b; > if (NILP (object)) > { > XSETBUFFER (object, current_buffer); > b = NULL; > } > else if (BUFFERP (object)) > b = XBUFFER (object); > [...] > signal_after_change (b, ...); I find the above much less readable than the current version, I must say. > > It was out of genuine interest, because passing NULL to implicitly > > specify a default argument is something that people advocate against, > > Not to specify the default, but to indicate that no action is needed > at all wrt the buffer. It is similar to the last argument to > 'strtol', for example. The `base' argument, you mean? If that's what you're saying, I agree that using 0 as a short-hand for "use implicit base" is an odd decision for C to have made, but I'm not sure I see the similarity to the current argument. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-17 11:38 ` Pip Cet @ 2019-06-17 15:59 ` Eli Zaretskii 2019-06-18 17:14 ` Pip Cet 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2019-06-17 15:59 UTC (permalink / raw) To: Pip Cet; +Cc: 36190 > From: Pip Cet <pipcet@gmail.com> > Date: Mon, 17 Jun 2019 11:38:38 +0000 > Cc: 36190@debbugs.gnu.org > > > Switching buffers means rebinding values of all the buffer-local > > variables, of which there could be quite a few. Or am I missing > > something? > > I just don't see how the requirement to switch buffers for modifying > text properties is so different, performance-wise, from the case of > modifying buffer text; in the latter case, we simply accept we can do > so only for the current buffer. It isn't different. It's just that (a) modifying another buffer's text is relatively rare, and (b) this is one more such switch. > In any case, the current code already switches buffers, it's just a > question of doing so twice rather than once. Yes. IOW, we get hit by that one more time. > > > > I wish we had a better alternative. > > > > > > (Such as not calling regular modification hooks for text property changes?) > > > > I thought about that, but I don't think this would be acceptable. > > It's certainly not something to be done on the spur of the moment, but > it is something I feel Emacs did wrongly, perhaps because XEmacs did > things differently, if I understand correctly. I'm not sure I'm aware > of even a single place where text properties are used for something > that's integrally part of buffer text. I don't think this i a part of the problem: applications that don't want the side effects of text properties can use overlays instead. > when someone has time to test things properly, is to rewrite all > buffer-modifying functions to look like this: > > Lisp_Object hooks = run_before_change_hooks (...); > modify_buffer (); > run_after_change_hooks (hooks, ...); I think that'd be a welcome refactoring, if indeed this paradigm doesn't break in some subtle use case (Emacs internals are frequently like that). > > struct buffer *b; > > if (NILP (object)) > > { > > XSETBUFFER (object, current_buffer); > > b = NULL; > > } > > else if (BUFFERP (object)) > > b = XBUFFER (object); > > [...] > > signal_after_change (b, ...); > > I find the above much less readable than the current version, I must say. I guess we will have to disagree then, because this is boilerplate C ion Emacs sources. > > > It was out of genuine interest, because passing NULL to implicitly > > > specify a default argument is something that people advocate against, > > > > Not to specify the default, but to indicate that no action is needed > > at all wrt the buffer. It is similar to the last argument to > > 'strtol', for example. > > The `base' argument, you mean? Sorry, meant the penultimate argument, ENDPTR. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' 2019-06-17 15:59 ` Eli Zaretskii @ 2019-06-18 17:14 ` Pip Cet 0 siblings, 0 replies; 22+ messages in thread From: Pip Cet @ 2019-06-18 17:14 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 36190 On Mon, Jun 17, 2019 at 3:58 PM Eli Zaretskii <eliz@gnu.org> wrote: > > From: Pip Cet <pipcet@gmail.com> > > Date: Mon, 17 Jun 2019 11:38:38 +0000 > > Cc: 36190@debbugs.gnu.org > > > > > I wish we had a better alternative. > > > > > > > > (Such as not calling regular modification hooks for text property changes?) > > > > > > I thought about that, but I don't think this would be acceptable. > > > > It's certainly not something to be done on the spur of the moment, but > > it is something I feel Emacs did wrongly, perhaps because XEmacs did > > things differently, if I understand correctly. I'm not sure I'm aware > > of even a single place where text properties are used for something > > that's integrally part of buffer text. > > I don't think this i a part of the problem: applications that don't > want the side effects of text properties can use overlays instead. I've seen performance problems with overlays, and the subtle API differences made switching, for me, difficult. You're right, though, that this is certainly not part of this bug report, which I think the latest patch I sent would fix for now (although I'd be happy to see it fixed any other way, too). > > when someone has time to test things properly, is to rewrite all > > buffer-modifying functions to look like this: > > > > Lisp_Object hooks = run_before_change_hooks (...); > > modify_buffer (); > > run_after_change_hooks (hooks, ...); > > I think that'd be a welcome refactoring, if indeed this paradigm > doesn't break in some subtle use case (Emacs internals are frequently > like that). It would almost certainly be an observable change in behavior, but I'm testing something and it doesn't seem to fall apart entirely, at least. > > > > It was out of genuine interest, because passing NULL to implicitly > > > > specify a default argument is something that people advocate against, > > > > > > Not to specify the default, but to indicate that no action is needed > > > at all wrt the buffer. It is similar to the last argument to > > > 'strtol', for example. > > > > The `base' argument, you mean? > > Sorry, meant the penultimate argument, ENDPTR. Thanks again for explaining, I'll think about it more. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-07-06 16:22 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-13 13:48 bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions' Pip Cet 2019-06-13 16:36 ` Eli Zaretskii 2019-06-13 18:48 ` Pip Cet 2019-06-13 19:05 ` Eli Zaretskii 2019-06-13 19:27 ` Eli Zaretskii 2019-06-13 19:42 ` Pip Cet 2019-06-13 20:01 ` Eli Zaretskii 2019-06-13 20:57 ` Pip Cet 2019-06-13 21:37 ` Pip Cet 2019-06-14 7:41 ` Eli Zaretskii 2019-06-14 11:14 ` Pip Cet 2019-06-14 12:10 ` Eli Zaretskii 2019-06-15 15:14 ` Pip Cet 2019-06-15 15:23 ` Eli Zaretskii 2019-06-15 19:27 ` Pip Cet 2019-07-06 8:08 ` Eli Zaretskii 2019-07-06 15:27 ` Pip Cet 2019-07-06 16:22 ` Eli Zaretskii 2019-06-14 7:36 ` Eli Zaretskii 2019-06-17 11:38 ` Pip Cet 2019-06-17 15:59 ` Eli Zaretskii 2019-06-18 17:14 ` Pip Cet
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.