Correctly implement dup and clone. Fixes #2627 #2694
Conversation
|
Nice work! Can we still define an abstract |
|
@jhass Nope, because abstract methods are checked by the compiler. I thought about adding some docs in |
| @@ -55,4 +55,9 @@ struct Bool | |||
| def to_s(io) | |||
| io << to_s | |||
| end | |||
|
|
|||
| # Returns `self` | |||
There was a problem hiding this comment.
I feel like the "Return self" comment everywhere should be a bit more verbose, pointing out that this is a Value type and thus assignment creates a copy.
Perhaps just "Returns self, assignment of this type creates a copy, see Value."
There was a problem hiding this comment.
If you don't want to expand them, best just drop them IMO.
There was a problem hiding this comment.
Oh, I forgot to enhance comments for clone. Do you think "Returns self (Bool is immutable)" and similar comments in other types would make this clearer?
There was a problem hiding this comment.
How about "Returns self, see Value#dup" ?
There was a problem hiding this comment.
But this is for clone, I'm not sure we should refer to dup. I think clone for primitive types is expected to return the same value. Maybe it's better to just mark it as :nodoc:? "Returns self" doesn't look bad to me.
There was a problem hiding this comment.
I wouldn't hide them from the API docs, but no comment is fine with me. "Returns self" is misleading if you come from a pass by reference only language IMO.
There was a problem hiding this comment.
Yeah, I guess no docs is fine too, one should expect Bool#clone to return a bool with the same value any way, and saying "returns self" can be misleading indeed.
Better than nothing! |
Is this safe (aka: if we move to a different GC or CoW strategy). Wouldn't be better avoid the low level aspect and perform a regular initialization with the same values? Pardon my ignorance regarding that item, but please feel free to correct me, will love to better understand pro vs cons vs portability. Thank you. |
|
@luislavena if we later change the GC strategy we can change that method too :-) A bigger worry is maybe |
asterite
force-pushed
the
feature/dup_clone
branch
from
861ae94 to
a0e21bf
Compare
|
I've updated the code a bit. I noticed Then I also mentioned why some |
asterite
force-pushed
the
feature/dup_clone
branch
from
a0e21bf to
e7bc74b
Compare
|
Removed all the "Return |
This implements all of the points from this comment, basically:
dupandclonefrom Objectdupfor Reference by memcpy of the underlying bytesdupfor Value by returningself(because that returns a copy)clonefor primitive types (Int,Float,Nil,Bool,Char) as returningself, because they are immutablecloneforReferenceorStruct: that's logic that is specific to each subclass. We of course implement it for some types in the standard library, likeArray,Hash,Set, etc.Additionally, there's a macro
Object#def_clonethat defines a clone method that invokescloneon all instance variables, which can be handy. The name is similar to other macros that define methods, likedef_equalsanddef_hash.This makes
dup's default behaviour consistent, as it returns a new, different object forReferencetypes (before this PR it would returnself).cloneis also improved: it doesn't exist by default so invokingcloneon a method that doesn't define it is a compile error, and at that point one must define its correct behaviour. So, this is a breaking change if someone was relying on this behaviour, but it's a breaking change that will likely spot bugs (or misuses ofclone, I found one in the compiler's source code).