GNU bug report logs - #20153
24.4.91; destructive add-face-text-property and string deep copying

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 20153 in the body.
You can then email your comments to 20153 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Oleh Krehel <ohwoeowho <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.4.91; destructive add-face-text-property and string deep copying
Date: Fri, 20 Mar 2015 13:28:00 +0100

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

Message #10 received at 20153 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Oleh Krehel <ohwoeowho <at> gmail.com>
Cc: 20153 <at> debbugs.gnu.org
Subject: Re: bug#20153: 24.4.91; destructive add-face-text-property and
 string deep copying
Date: Wed, 09 Oct 2019 04:25:04 +0200

Oleh Krehel <ohwoeowho <at> 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

Message #13 received at 20153 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Oleh Krehel <ohwoeowho <at> gmail.com>
Cc: 20153 <at> debbugs.gnu.org
Subject: Re: bug#20153: 24.4.91; destructive add-face-text-property and
 string deep copying
Date: Wed, 09 Oct 2019 05:04:03 +0200

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

Message #20 received at 20153 <at> debbugs.gnu.org (full text, mbox):

From: Oleh Krehel <ohwoeowho <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 20153 <at> debbugs.gnu.org
Subject: Re: bug#20153: 24.4.91; destructive add-face-text-property and string
 deep copying
Date: Wed, 9 Oct 2019 10:24:40 +0200

Thanks for your work, Lars.

  Oleh

Message #23 received at 20153 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 20153 <at> debbugs.gnu.org, ohwoeowho <at> gmail.com
Subject: Re: bug#20153: 24.4.91;
 destructive add-face-text-property and string deep copying
Date: Wed, 09 Oct 2019 20:10:16 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Wed, 09 Oct 2019 05:04:03 +0200
> Cc: 20153 <at> 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?

Message #26 received at 20153 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 20153 <at> debbugs.gnu.org, ohwoeowho <at> gmail.com
Subject: Re: bug#20153: 24.4.91; destructive add-face-text-property and
 string deep copying
Date: Wed, 09 Oct 2019 19:45:54 +0200

Eli Zaretskii <eliz <at> 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

Message #29 received at 20153 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 20153 <at> debbugs.gnu.org, ohwoeowho <at> gmail.com
Subject: Re: bug#20153: 24.4.91; destructive add-face-text-property and
 string deep copying
Date: Wed, 09 Oct 2019 21:06:54 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: ohwoeowho <at> gmail.com,  20153 <at> debbugs.gnu.org
> Date: Wed, 09 Oct 2019 19:45:54 +0200
> 
> Eli Zaretskii <eliz <at> 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.

Message #32 received at 20153 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 20153 <at> debbugs.gnu.org, ohwoeowho <at> gmail.com
Subject: Re: bug#20153: 24.4.91; destructive add-face-text-property and
 string deep copying
Date: Wed, 09 Oct 2019 20:09:13 +0200

Eli Zaretskii <eliz <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.