lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Copy alist instead of deep copy on Grob clone (issue 561640045 by ad


From: jonas . hahnfeld
Subject: Re: Copy alist instead of deep copy on Grob clone (issue 561640045 by address@hidden)
Date: Tue, 14 Apr 2020 13:07:16 -0700

On 2020/04/14 19:14:29, hanwenn wrote:
> On 2020/04/14 10:22:01, hahnjo wrote:
> > On 2020/04/14 09:39:18, hanwenn wrote:
> > > it reads from the immutable_list_ if there is no override in the
> > > mutable property list.
> > 
> > Ack, that's also what the two names suggested. The code in Grob's
copy
> > constructor does a copy of mutable_property_alist_. As far as I
understand it
> > contains those elements that have been changed for the Grob? So I'm
thinking
> > about the following situation (not real code, just conceptually):
> > Grob g1;
> > g1.set_property("key", "value");
> > Grob g2(g1);
> > g2.set_property("key", "value2");
> > 
> > set_property will change mutable_property_alist_, and with this
change both
> > Grobs will share some of it. So I can't help, but this looks like
the
> > g1.get_property("key") will probably return "value2" at the end, no?
> 
> The code now calls ly_alist_copy(), which constructs a new alist with
the same
> keys and values,
> so your scenario will continue to work correctly.
> 
> A deep copy protects against something like
> 
>  Grob g1;
>  g1.set_property("key", scm_cons(1,2));
>  Grob g2(g1);
>  SCM v = g2.get_property("key");
>  scm_set_cdr_x(v, 3)
> 
> but this protection only works if the set_property was called
beforehand. If you
> do this for a something that comes out of the immutable list, it'll be
a
> disaster.

My point is that there is some shared memory by two different Grobs.
This can potentially lead to problems and very surprising behavior.
Given that there's no measurable performance impact, I remain opposed to
this change.

https://codereview.appspot.com/561640045/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]