New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expression node cloning isn't a deep copy #745

Closed
evykassirer opened this Issue Nov 9, 2016 · 9 comments

Comments

Projects
None yet
3 participants
@evykassirer

evykassirer commented Nov 9, 2016

x = math.parse('2+2+3');
y = x.clone()

case 1:

x.args[1] = math.parse('555')
x.toString()
> "2 + 2 + 555"
y.toString()
> "2 + 2 + 3"

case 2:

x.args[0].args[0] = math.parse('56')
x.toString()
> "56 + 2 + 3"
y.toString()
> "56 + 2 + 3"

clone() is supposed to recursively copy (deep copy), right?

so this seems like a bug 😞

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Nov 10, 2016

The code suggests that it is supposed to be a shallow clone.

@FSMaxB FSMaxB added the question label Nov 10, 2016

@evykassirer

This comment has been minimized.

evykassirer commented Nov 10, 2016

The documentation says it's a recursive clone, so one or the other should change.

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Nov 10, 2016

The documentation was changed here and at the time it was recursive. It was changed to be a shallow clone in this commit. Not sure why.

@FSMaxB FSMaxB added bug and removed question labels Nov 10, 2016

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 10, 2016

Hm. I agree this is counter intuitive. I guess I changed it because of performance reasons or something.

Maybe we should change it to a deep copy again, with an optional flag to do a shallow clone (or a separate method).

@evykassirer

This comment has been minimized.

evykassirer commented Nov 10, 2016

found another issue with clone() - it doesn't preserve implicit multiplication in its cloning

x.toString() => 2 x
y = x.clone()
y.toString() => 2*x

Probably matters less to y'all, but it's important to the pedagogy of our math stepper.
For now, I'm accurately deep cloning by printing and reparsing, so this change isn't urgent, but it'd defs be nice :)

@FSMaxB

This comment has been minimized.

Collaborator

FSMaxB commented Nov 11, 2016

Easy fix, see #747. Now I'm just waiting for Travis to run the tests.

@evykassirer

This comment has been minimized.

evykassirer commented Nov 11, 2016

Beautiful :) thanks

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 18, 2016

I've implemented a new method Node.cloneDeep.

So now we have a Node.clone() (shallow) and Node.cloneDeep() (deep), similar to libraries like lodash.

Will do a release tonight.

@josdejong

This comment has been minimized.

Owner

josdejong commented Nov 18, 2016

I've released v3.8.0 containing these fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment