* bug#20153: 24.4.91; destructive add-face-text-property and string deep copying @ 2015-03-20 12:28 Oleh Krehel 2019-10-09 2:25 ` Lars Ingebrigtsen 2019-10-09 3:04 ` Lars Ingebrigtsen 0 siblings, 2 replies; 8+ messages in thread From: Oleh Krehel @ 2015-03-20 12:28 UTC (permalink / raw) To: 20153 Here's an example of what I'm trying to do: (setq test-str-1 #(";; This `is' a test" 0 3 (fontified nil face font-lock-comment-delimiter-face) 3 9 (fontified nil face font-lock-comment-face) 9 11 (fontified nil face (font-lock-constant-face font-lock-comment-face)) 11 19 (fontified nil face font-lock-comment-face))) (setq test-str-2 (concat test-str-1)) (add-face-text-property 0 (length test-str-2) 'foobar t test-str-2) I would like to modify `test-str-2' without touching `test-str-1'. For that, I either need `add-face-text-property' to be non-destructive, either w.r.t. the string or at least w.r.t. the properties of the string, or a way to deep-copy a string. Here's the result of evaling the code above: test-str-2 ;; => #(";; This `is' a test" 0 3 (fontified nil face (font-lock-comment-delimiter-face foobar)) 3 9 (fontified nil face (font-lock-comment-face foobar)) 9 11 (fontified nil face (font-lock-constant-face font-lock-comment-face foobar)) 11 19 (fontified nil face (font-lock-comment-face foobar))) test-str-1 ;; => #(";; This `is' a test" 0 3 (face font-lock-comment-delimiter-face fontified nil) 3 9 (face font-lock-comment-face fontified nil) 9 11 (face (font-lock-constant-face font-lock-comment-face foobar) ; <= foobar is here fontified nil) 11 19 (face font-lock-comment-face fontified nil)) As you see, `test-str-1' was modified. I'd like to know a straightforward solution to the problem itself, I'm fine with either way of implementation. Oleh ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#20153: 24.4.91; destructive add-face-text-property and string deep copying 2015-03-20 12:28 bug#20153: 24.4.91; destructive add-face-text-property and string deep copying Oleh Krehel @ 2019-10-09 2:25 ` Lars Ingebrigtsen 2019-10-09 3:04 ` Lars Ingebrigtsen 1 sibling, 0 replies; 8+ messages in thread From: Lars Ingebrigtsen @ 2019-10-09 2:25 UTC (permalink / raw) To: Oleh Krehel; +Cc: 20153 Oleh Krehel <ohwoeowho@gmail.com> writes: > Here's an example of what I'm trying to do: > > (setq test-str-1 > #(";; This `is' a test" > 0 3 (fontified nil face font-lock-comment-delimiter-face) > 3 9 (fontified nil face font-lock-comment-face) > 9 11 (fontified nil face (font-lock-constant-face font-lock-comment-face)) > 11 19 (fontified nil face font-lock-comment-face))) > (setq test-str-2 (concat test-str-1)) > (add-face-text-property 0 (length test-str-2) 'foobar t test-str-2) > > I would like to modify `test-str-2' without touching `test-str-1'. > For that, I either need `add-face-text-property' to be non-destructive, > either w.r.t. the string or at least w.r.t. the properties of the > string, or a way to deep-copy a string. It's this code (in add_properties): /* The previous value is a list, so prepend (or append) the new value to this list. */ if (set_type == TEXT_PROPERTY_PREPEND) Fsetcar (this_cdr, Fcons (val1, Fcar (this_cdr))); else nconc2 (Fcar (this_cdr), list1 (val1)); But changing that would have pretty far-reaching consequences. I wonder whether instead Fadd_face_text_property should just (when called with a string parameter) just copy the `face' property list first? That should avoid the problem and not lead to any general slowdown. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#20153: 24.4.91; destructive add-face-text-property and string deep copying 2015-03-20 12:28 bug#20153: 24.4.91; destructive add-face-text-property and string deep copying Oleh Krehel 2019-10-09 2:25 ` Lars Ingebrigtsen @ 2019-10-09 3:04 ` Lars Ingebrigtsen 2019-10-09 8:24 ` Oleh Krehel 2019-10-09 17:10 ` Eli Zaretskii 1 sibling, 2 replies; 8+ messages in thread From: Lars Ingebrigtsen @ 2019-10-09 3:04 UTC (permalink / raw) To: Oleh Krehel; +Cc: 20153 I've never really worked with the interval internals before, so I thought this was going to be easy to fix. :-/ But the problem is that copy_intervals doesn't do a "deep" copy of the text properties, so this has no effect, really. (Patch included for reference.) Instead I've now changed add_properties (and add_text_properties_1) to take a bool parameter to say whether they're allowed to be destructive or not, and make the add-face-text-property call that with false as the parameter if the object is a string. This fixes the test case for me and should hopefully have no measurable performance impact. diff --git a/src/textprop.c b/src/textprop.c index d36b9e14a6..dcd3284209 100644 --- a/src/textprop.c +++ b/src/textprop.c @@ -1334,6 +1334,18 @@ face(s) are retained. This is done by setting the `face' property to Lisp_Object append, Lisp_Object object) { AUTO_LIST2 (properties, Qface, face); + + /* If we're adding face properties to a string, and the face + property is already a list, then copy the list first to avoid + destructively altering it. */ + if (STRINGP (object)) + { + INTERVAL copy = copy_intervals (string_intervals (object), + 0, SCHARS (object)); + set_interval_object (copy, object); + set_string_intervals (object, copy); + } + add_text_properties_1 (start, end, properties, object, (NILP (append) ? TEXT_PROPERTY_PREPEND -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#20153: 24.4.91; destructive add-face-text-property and string deep copying 2019-10-09 3:04 ` Lars Ingebrigtsen @ 2019-10-09 8:24 ` Oleh Krehel 2019-10-09 17:10 ` Eli Zaretskii 1 sibling, 0 replies; 8+ messages in thread From: Oleh Krehel @ 2019-10-09 8:24 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 20153 Thanks for your work, Lars. Oleh ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#20153: 24.4.91; destructive add-face-text-property and string deep copying 2019-10-09 3:04 ` Lars Ingebrigtsen 2019-10-09 8:24 ` Oleh Krehel @ 2019-10-09 17:10 ` Eli Zaretskii 2019-10-09 17:45 ` Lars Ingebrigtsen 1 sibling, 1 reply; 8+ messages in thread From: Eli Zaretskii @ 2019-10-09 17:10 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 20153, ohwoeowho > From: Lars Ingebrigtsen <larsi@gnus.org> > Date: Wed, 09 Oct 2019 05:04:03 +0200 > Cc: 20153@debbugs.gnu.org > > Instead I've now changed add_properties (and add_text_properties_1) to > take a bool parameter to say whether they're allowed to be destructive > or not, and make the add-face-text-property call that with false as the > parameter if the object is a string. This fixes the test case for me > and should hopefully have no measurable performance impact. Isn't that a backward-incompatible change? ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#20153: 24.4.91; destructive add-face-text-property and string deep copying 2019-10-09 17:10 ` Eli Zaretskii @ 2019-10-09 17:45 ` Lars Ingebrigtsen 2019-10-09 18:06 ` Eli Zaretskii 0 siblings, 1 reply; 8+ messages in thread From: Lars Ingebrigtsen @ 2019-10-09 17:45 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 20153, ohwoeowho Eli Zaretskii <eliz@gnu.org> writes: >> Instead I've now changed add_properties (and add_text_properties_1) to >> take a bool parameter to say whether they're allowed to be destructive >> or not, and make the add-face-text-property call that with false as the >> parameter if the object is a string. This fixes the test case for me >> and should hopefully have no measurable performance impact. > > Isn't that a backward-incompatible change? It is, but the previous behaviour was a bug. If you have a copy of a string and modify the copy, the original string shouldn't change. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#20153: 24.4.91; destructive add-face-text-property and string deep copying 2019-10-09 17:45 ` Lars Ingebrigtsen @ 2019-10-09 18:06 ` Eli Zaretskii 2019-10-09 18:09 ` Lars Ingebrigtsen 0 siblings, 1 reply; 8+ messages in thread From: Eli Zaretskii @ 2019-10-09 18:06 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 20153, ohwoeowho > From: Lars Ingebrigtsen <larsi@gnus.org> > Cc: ohwoeowho@gmail.com, 20153@debbugs.gnu.org > Date: Wed, 09 Oct 2019 19:45:54 +0200 > > Eli Zaretskii <eliz@gnu.org> writes: > > >> Instead I've now changed add_properties (and add_text_properties_1) to > >> take a bool parameter to say whether they're allowed to be destructive > >> or not, and make the add-face-text-property call that with false as the > >> parameter if the object is a string. This fixes the test case for me > >> and should hopefully have no measurable performance impact. > > > > Isn't that a backward-incompatible change? > > It is, but the previous behaviour was a bug. If you have a copy of a > string and modify the copy, the original string shouldn't change. AFAIU, the string didn't change, only its plist did. Right? Please bring this up on emacs-devel, and if no one objects to the change, we should at least mention it in NEWS. Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#20153: 24.4.91; destructive add-face-text-property and string deep copying 2019-10-09 18:06 ` Eli Zaretskii @ 2019-10-09 18:09 ` Lars Ingebrigtsen 0 siblings, 0 replies; 8+ messages in thread From: Lars Ingebrigtsen @ 2019-10-09 18:09 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 20153, ohwoeowho Eli Zaretskii <eliz@gnu.org> writes: > AFAIU, the string didn't change, only its plist did. Right? Yes. > Please bring this up on emacs-devel, and if no one objects to the > change, we should at least mention it in NEWS. Will do. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-10-09 18:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-20 12:28 bug#20153: 24.4.91; destructive add-face-text-property and string deep copying Oleh Krehel 2019-10-09 2:25 ` Lars Ingebrigtsen 2019-10-09 3:04 ` Lars Ingebrigtsen 2019-10-09 8:24 ` Oleh Krehel 2019-10-09 17:10 ` Eli Zaretskii 2019-10-09 17:45 ` Lars Ingebrigtsen 2019-10-09 18:06 ` Eli Zaretskii 2019-10-09 18:09 ` Lars Ingebrigtsen
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.