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:jonas.hahnf...@gmail.com> 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/
