Re: Copy alist instead of deep copy on Grob clone (issue 561640045 by address@hidden)

[hanwenn]

On 2020/04/14 10:22:01, hahnjo wrote:
> On 2020/04/14 09:39:18, hanwenn wrote:
> > On Tue, Apr 14, 2020 at 9:28 AM <mailto:address@hidden>
wrote:
> > >
> > > On 2020/04/14 07:24:10, hanwenn wrote:
> > > > On 2020/04/14 07:00:56, hahnjo wrote:
> > > > > On 2020/04/13 21:01:11, hanwenn wrote:
> > > > > > it's hard to say if this makes a measurable difference:
> > > > > >
> > > > > > [...]
> > > > >
> > > > > So then ... why do it at all? From a code perspective, a deep
copy
> > > looks saner
> > > > > in terms of "encapsulation" (one principle of object-oriented
> > > programming). If
> > > > > there is no provable gain, I'm for leaving the current code
alone
> > > until there
> > > > is
> > > > > good reason to change - "premature optimization is the root of
all
> > > evil"
> > > >
> > > > As I described in the commit message, this code doesn't protect
> > > anything,
> > > > because get_property can also return values from the immutable
list.
> > >
> > > get_property uses data backed by mutable_property_alist_, right?
> > 
> > Use the Source, Jonas!
> 
> I find this statement inappropriate: I'm not going to read through
100k LOC to
> find the one thing that is possibly different from what I remember.
That is why
> I ask if I'm missing something.

Sorry!

>
https://github.com/lilypond/lilypond/blob/0c00cd98e81b27325bed5891b950fe7f0f0ebe5d/lily/grob-property.cc#L140
> > 
> > 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.



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