From eeaa488bf8fe4f2e8eb887faa0d411e90972a970 Mon Sep 17 00:00:00 2001 From: Stefan Monnier Date: Sun, 23 Feb 2025 00:29:49 -0500 Subject: [PATCH] (Ftranspose_regions): Fix text-properties for len1==len2 When `len1_byte == len2_byte`, the code presumed that len1==len2 as well in its handling of text-properties. Fix that case. While at it, try and reduce code duplication by hoisting common code out of `if`s, and throw away the optimization for `len_mid == 0` which only saved 3 trivial function calls. * src/editfns.c (Ftranspose_regions): Shuffle the code a bit. * test/src/editfns-tests.el (editfns-tests--transpose-equal-but-not): New test. (cherry picked from commit d84dbcb4504f6c53968a9f245b31676c90921b38) --- src/editfns.c | 200 ++++++++++++-------------------------- test/src/editfns-tests.el | 15 +++ 2 files changed, 75 insertions(+), 140 deletions(-) diff --git a/src/editfns.c b/src/editfns.c index 8fe2ecf1a03..3dff49fb00c 100644 --- a/src/editfns.c +++ b/src/editfns.c @@ -4423,7 +4423,7 @@ ring. */) ptrdiff_t gap, len1, len_mid, len2; unsigned char *start1_addr, *start2_addr, *temp; - INTERVAL cur_intv, tmp_interval1, tmp_interval_mid, tmp_interval2, tmp_interval3; + INTERVAL cur_intv, tmp_interval1, tmp_interval2, tmp_interval3; Lisp_Object buf; XSETBUFFER (buf, current_buffer); @@ -4494,7 +4494,8 @@ ring. */) } start2_byte = CHAR_TO_BYTE (start2); - len1_byte = CHAR_TO_BYTE (end1) - start1_byte; + ptrdiff_t end1_byte = CHAR_TO_BYTE (end1); + len1_byte = end1_byte - start1_byte; len2_byte = end2_byte - start2_byte; #ifdef BYTE_COMBINING_DEBUG @@ -4526,168 +4527,87 @@ ring. */) enough to use as the temporary storage? That would avoid an allocation... interesting. Later, don't fool with it now. */ - if (end1 == start2) /* adjacent regions */ + modify_text (start1, end2); + tmp_interval1 = copy_intervals (cur_intv, start1, len1); + tmp_interval2 = copy_intervals (cur_intv, start2, len2); + USE_SAFE_ALLOCA; + if (len1_byte == len2_byte && len1 == len2) + /* Regions are same size, though, how nice. */ + /* The char lengths also have to match, for text-properties. */ { - modify_text (start1, end2); - record_change (start1, len1 + len2); + if (end1 == start2) /* Merge the two parts into a single one. */ + record_change (start1, (end2 - start1)); + else + { + record_change (start1, len1); + record_change (start2, len2); + } - tmp_interval1 = copy_intervals (cur_intv, start1, len1); - tmp_interval2 = copy_intervals (cur_intv, start2, len2); - /* Don't use Fset_text_properties: that can cause GC, which can - clobber objects stored in the tmp_intervals. */ - tmp_interval3 = validate_interval_range (buf, &startr1, &endr2, 0); + tmp_interval3 = validate_interval_range (buf, &startr1, &endr1, 0); if (tmp_interval3) - set_text_properties_1 (startr1, endr2, Qnil, buf, tmp_interval3); - - USE_SAFE_ALLOCA; + set_text_properties_1 (startr1, endr1, Qnil, buf, tmp_interval3); - /* First region smaller than second. */ - if (len1_byte < len2_byte) - { - temp = SAFE_ALLOCA (len2_byte); - - /* Don't precompute these addresses. We have to compute them - at the last minute, because the relocating allocator might - have moved the buffer around during the xmalloc. */ - start1_addr = BYTE_POS_ADDR (start1_byte); - start2_addr = BYTE_POS_ADDR (start2_byte); - - memcpy (temp, start2_addr, len2_byte); - memcpy (start1_addr + len2_byte, start1_addr, len1_byte); - memcpy (start1_addr, temp, len2_byte); - } - else - /* First region not smaller than second. */ - { - temp = SAFE_ALLOCA (len1_byte); - start1_addr = BYTE_POS_ADDR (start1_byte); - start2_addr = BYTE_POS_ADDR (start2_byte); - memcpy (temp, start1_addr, len1_byte); - memcpy (start1_addr, start2_addr, len2_byte); - memcpy (start1_addr + len2_byte, temp, len1_byte); - } - - SAFE_FREE (); - graft_intervals_into_buffer (tmp_interval1, start1 + len2, - len1, current_buffer, 0); - graft_intervals_into_buffer (tmp_interval2, start1, - len2, current_buffer, 0); - update_compositions (start1, start1 + len2, CHECK_BORDER); - update_compositions (start1 + len2, end2, CHECK_TAIL); + tmp_interval3 = validate_interval_range (buf, &startr2, &endr2, 0); + if (tmp_interval3) + set_text_properties_1 (startr2, endr2, Qnil, buf, tmp_interval3); + + temp = SAFE_ALLOCA (len1_byte); + start1_addr = BYTE_POS_ADDR (start1_byte); + start2_addr = BYTE_POS_ADDR (start2_byte); + memcpy (temp, start1_addr, len1_byte); + memcpy (start1_addr, start2_addr, len2_byte); + memcpy (start2_addr, temp, len1_byte); } - /* Non-adjacent regions, because end1 != start2, bleagh... */ else { - len_mid = start2_byte - (start1_byte + len1_byte); - - if (len1_byte == len2_byte) - /* Regions are same size, though, how nice. */ - { - USE_SAFE_ALLOCA; - - modify_text (start1, end2); - record_change (start1, len1); - record_change (start2, len2); - tmp_interval1 = copy_intervals (cur_intv, start1, len1); - tmp_interval2 = copy_intervals (cur_intv, start2, len2); - - tmp_interval3 = validate_interval_range (buf, &startr1, &endr1, 0); - if (tmp_interval3) - set_text_properties_1 (startr1, endr1, Qnil, buf, tmp_interval3); - - tmp_interval3 = validate_interval_range (buf, &startr2, &endr2, 0); - if (tmp_interval3) - set_text_properties_1 (startr2, endr2, Qnil, buf, tmp_interval3); - - temp = SAFE_ALLOCA (len1_byte); - start1_addr = BYTE_POS_ADDR (start1_byte); - start2_addr = BYTE_POS_ADDR (start2_byte); - memcpy (temp, start1_addr, len1_byte); - memcpy (start1_addr, start2_addr, len2_byte); - memcpy (start2_addr, temp, len1_byte); - SAFE_FREE (); - - graft_intervals_into_buffer (tmp_interval1, start2, - len1, current_buffer, 0); - graft_intervals_into_buffer (tmp_interval2, start1, - len2, current_buffer, 0); - } - - else if (len1_byte < len2_byte) /* Second region larger than first */ - /* Non-adjacent & unequal size, area between must also be shifted. */ - { - USE_SAFE_ALLOCA; - - modify_text (start1, end2); - record_change (start1, (end2 - start1)); - tmp_interval1 = copy_intervals (cur_intv, start1, len1); - tmp_interval_mid = copy_intervals (cur_intv, end1, len_mid); - tmp_interval2 = copy_intervals (cur_intv, start2, len2); - - tmp_interval3 = validate_interval_range (buf, &startr1, &endr2, 0); - if (tmp_interval3) - set_text_properties_1 (startr1, endr2, Qnil, buf, tmp_interval3); - + len_mid = start2_byte - end1_byte; + record_change (start1, (end2 - start1)); + INTERVAL tmp_interval_mid = copy_intervals (cur_intv, end1, len_mid); + tmp_interval3 = validate_interval_range (buf, &startr1, &endr2, 0); + if (tmp_interval3) + set_text_properties_1 (startr1, endr2, Qnil, buf, tmp_interval3); + if (len1_byte < len2_byte) /* Second region larger than first */ + { /* holds region 2 */ temp = SAFE_ALLOCA (len2_byte); start1_addr = BYTE_POS_ADDR (start1_byte); start2_addr = BYTE_POS_ADDR (start2_byte); - memcpy (temp, start2_addr, len2_byte); - memcpy (start1_addr + len_mid + len2_byte, start1_addr, len1_byte); - memmove (start1_addr + len2_byte, start1_addr + len1_byte, len_mid); - memcpy (start1_addr, temp, len2_byte); - SAFE_FREE (); - - graft_intervals_into_buffer (tmp_interval1, end2 - len1, - len1, current_buffer, 0); - graft_intervals_into_buffer (tmp_interval_mid, start1 + len2, - len_mid, current_buffer, 0); - graft_intervals_into_buffer (tmp_interval2, start1, - len2, current_buffer, 0); - } + memcpy (temp, start2_addr, len2_byte); + memcpy (start1_addr + len_mid + len2_byte, start1_addr, len1_byte); + memmove (start1_addr + len2_byte, start1_addr + len1_byte, len_mid); + memcpy (start1_addr, temp, len2_byte); + } else /* Second region smaller than first. */ - { - USE_SAFE_ALLOCA; - - record_change (start1, (end2 - start1)); - modify_text (start1, end2); - - tmp_interval1 = copy_intervals (cur_intv, start1, len1); - tmp_interval_mid = copy_intervals (cur_intv, end1, len_mid); - tmp_interval2 = copy_intervals (cur_intv, start2, len2); - - tmp_interval3 = validate_interval_range (buf, &startr1, &endr2, 0); - if (tmp_interval3) - set_text_properties_1 (startr1, endr2, Qnil, buf, tmp_interval3); - + { /* holds region 1 */ temp = SAFE_ALLOCA (len1_byte); start1_addr = BYTE_POS_ADDR (start1_byte); start2_addr = BYTE_POS_ADDR (start2_byte); - memcpy (temp, start1_addr, len1_byte); - memcpy (start1_addr, start2_addr, len2_byte); - memmove (start1_addr + len2_byte, start1_addr + len1_byte, len_mid); - memcpy (start1_addr + len2_byte + len_mid, temp, len1_byte); - SAFE_FREE (); - - graft_intervals_into_buffer (tmp_interval1, end2 - len1, - len1, current_buffer, 0); - graft_intervals_into_buffer (tmp_interval_mid, start1 + len2, - len_mid, current_buffer, 0); - graft_intervals_into_buffer (tmp_interval2, start1, - len2, current_buffer, 0); - } - - update_compositions (start1, start1 + len2, CHECK_BORDER); - update_compositions (end2 - len1, end2, CHECK_BORDER); + memcpy (temp, start1_addr, len1_byte); + memcpy (start1_addr, start2_addr, len2_byte); + memmove (start1_addr + len2_byte, start1_addr + len1_byte, len_mid); + memcpy (start1_addr + len2_byte + len_mid, temp, len1_byte); + } + graft_intervals_into_buffer (tmp_interval_mid, start1 + len2, + len_mid, current_buffer, 0); } + SAFE_FREE (); + graft_intervals_into_buffer (tmp_interval1, end2 - len1, + len1, current_buffer, 0); + graft_intervals_into_buffer (tmp_interval2, start1, + len2, current_buffer, 0); + + update_compositions (start1, start1 + len2, CHECK_BORDER); + update_compositions (end2 - len1, end2, CHECK_BORDER); /* When doing multiple transpositions, it might be nice to optimize this. Perhaps the markers in any one buffer should be organized in some sorted data tree. */ if (NILP (leave_markers)) { + /* FIXME: Since the undo info doesn't record the transposition as its own + operation, we won't enjoy 'transpose_markers' during undo :-( */ transpose_markers (start1, end1, start2, end2, start1_byte, start1_byte + len1_byte, start2_byte, start2_byte + len2_byte); diff --git a/test/src/editfns-tests.el b/test/src/editfns-tests.el index 8d4e7bc48fa..29b7a850838 100644 --- a/test/src/editfns-tests.el +++ b/test/src/editfns-tests.el @@ -175,6 +175,21 @@ (should (string= (buffer-string) "éä\"ba÷")) (should (equal (transpose-test-get-byte-positions 7) '(1 3 5 6 7 8 10))))) +(ert-deftest editfns-tests--transpose-equal-but-not () + (with-temp-buffer + (let ((str1 (propertize "ab" 'my-prop 'ab)) + (str2 (propertize "SPC" 'my-prop 'SPC)) + (str3 (propertize "é" 'my-prop 'é))) + (insert " " str1 str2 str3 " ") + (transpose-regions (+ (point-min) 1) (+ (point-min) 3) + (+ (point-min) 6) (+ (point-min) 7)) + (should (equal-including-properties + str3 (buffer-substring (+ (point-min) 1) (+ (point-min) 2)))) + (should (equal-including-properties + str2 (buffer-substring (+ (point-min) 2) (+ (point-min) 5)))) + (should (equal-including-properties + str1 (buffer-substring (+ (point-min) 5) (+ (point-min) 7))))))) + (ert-deftest format-c-float () (should-error (format "%c" 0.5))) -- 2.39.5