Skip to content
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

remove some unncessary and/or misleading methods of copy #15675

Merged
merged 1 commit into from
Apr 25, 2016

Conversation

JeffBezanson
Copy link
Member

I think it's better not to define copy on objects where it doesn't make sense and doesn't do anything. IIRC, most of these definitions were introduced a long time ago when they might have been needed for copying ASTs. That was silly, and has been replaced by the internal astcopy function that knows how to copy only what's needed.

These can cause problems, such as writing copy(str).data and thinking you'll get a new copy of the string's data.

To minimize breakage, I'm leaving the definitions for Range and Number for now, since there is some code that assumes all array-like things are copyable.

@tkelman
Copy link
Contributor

tkelman commented Mar 29, 2016

@jrevels how far are we from hooking up automatic pkgeval on demand?

@jrevels
Copy link
Member

jrevels commented Mar 29, 2016

how far are we from hooking up automatic pkgeval on demand?

It's not something I've worked on, besides separating out the CI bot code from the benchmarking code. After this week, my paper-crunch should be over, and I'll finally have time to finish the current iteration of our benchmark tools. After that, I'll be working on the CI stuff again. I was going to start with setting up a database for the benchmark data, but it seems like getting the CI bot to run pkgeval is more urgent, so I could refocus and prioritize that instead.

@tkelman
Copy link
Contributor

tkelman commented Mar 29, 2016

Good to know, thanks. PkgEval can be run manually anywhere we think we need it for now, and abusing git instead of using a real database has worked for the pkg.julialang.org results for a while. Either would be useful.

@JeffBezanson JeffBezanson merged commit bdafea9 into master Apr 25, 2016
@yuyichao yuyichao deleted the jb/lesscopy branch April 25, 2016 19:37
@timholy
Copy link
Member

timholy commented Apr 26, 2016

This should have had some kind of deprecation warning, rather than just removing the methods.

JuliaImages/Images.jl#478.

@timholy
Copy link
Member

timholy commented Apr 26, 2016

Also needs fixing:

R = copy(region)

(perhaps among others)

@JeffBezanson
Copy link
Member Author

@timholy What was being copied in Images?

@timholy
Copy link
Member

timholy commented Apr 26, 2016

A string, https://github.com/timholy/Images.jl/blob/531bd0131079dd59db44ffdf8caeede81e7fd291/src/core.jl#L156-L160.

If you want to trigger the second error, you should be able to just replace dictcopy with deepcopy (now that it's type-stable) and then run the Images tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants