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/
