unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* 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 public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).