Fix .clone() when providing new array/hash attributes #170
Conversation
|
I'd really like to understand the semantics of .clone before moving too far ahead on this patch (e.g,. some clear examples and perhaps pointers to relevant spec sections). In particular, this patch seems to add (deep) cloning of hash and array attributes as well as scalars. But then why do the test at all? Are there specific attributes that shouldn't be cloned, and if so, what are they? Pm |
|
@pmichaud thanks for taking the time to look at this. The clone section of the spec is pretty sparse. http://perlcabal.org/syn/S12.html#Cloning I am trying to replicate what seems like reasonable functionality in this patch. That being: cloning an instance of class, and mutating attributes of that clone does not affect the original object. I have implemented tests in roast in the following file (link to the lines relevant to this post) https://github.com/perl6/roast/blob/master/S12-attributes/clone.t#L69 There does appear to be some attributes which shouldn't be cloned. Specifically, in Rakudo, scalar attributes which are decontainerized seem to not want to be cloned (maybe @sorear or @jnthn or @masak could weigh in to as to why, I don't fully understand why not). Rakudo emits an exception when attempting to clone those objects. The primary example of this, which is tested in roast, is cloning match objects. Note that Capture's |
|
If At any rate, if it makes tests pass I'm okay with the pull request for now... I just don't think it's likely to be the long-term solution. Pm |
|
Ok, we can leave the ticket open and I can write up your comments in that ticket. Merging this does make the tests pasts, and makes it behave like niecza (which also passes the tests). Leaving a note in that ticket so we can revisit when appropriate. |
|
Actually, I think I've found a better way. Going to push it here in a few minutes. |
this undoes a prior commit with a better version. now array/hash attributes will not be deep-copied. the test suite S12-attributes/clone.t has been updated with much more coverage. this commit also brings it inline with niecza's behavior.
|
@pmichaud just pushed a change which I think works better. I added a bunch more coverage to S12-attributes/clone.t - testing the behavior of deep copying [no deep copying] and copying objects in addition to arrays/hashes [same]. This patch makes everything pass, and have the same behavior of niecza (which appears sane, correct, and mirrored in the tests). |
| } | ||
| # we don't want to update the original hash/array because a new one was supplied, | ||
| # create a new one to hold the value and bind the attribute | ||
| if is_kind('@') { |
There was a problem hiding this comment.
should } elsif ($name ~~ Array) {, methinks
There was a problem hiding this comment.
This doesn't work because $name is not a positional or hash... it is the name of the attribute. Therefore, we never fall into this branch, and values are not coerced correctly.
|
Can this pull request be closed now? |
|
I think so, since it already fixed. |
|
I kindly remind that Bug#118559 is marked as closed :) |
|
Based on comments from 2015, closing this PR. Thanks. |
This fixes https://rt.perl.org/rt3/Ticket/Display.html?id=118559, passing spectests.
It accomplishes this by cloning hash and array attributes, identified by their sigil, when deciding whether to clone attributes.It accomplishes this by ensuring that new lists are assigned to the new object rather than updating that objects list, which points at the list of the original object.