Conversation
g11tech
left a comment
g11tech
left a comment
There was a problem hiding this comment.
check if there there is a variable type size list, add it in the test case if not already
gballet
left a comment
gballet
left a comment
There was a problem hiding this comment.
I don't think this should be in this library, this is just a generic cloning feature. In fact, there is no need to call the function sszClone, a simple clone is enough. Then the semantics are the same for everything, because there is no reason for several clone functions to exist in any given object.
So I suggest moving all of that code to zeam, except the sszClone functions in utils.zig that should remain in this PR (which should be renamed clone).
| /// borrowed `[]const u8` data remains borrowed. Types providing `sszClone` | ||
| /// must also provide `deinit`; partial-failure unwinds rely on `deinit` to | ||
| /// release whatever the hook allocated. | ||
| pub fn clone(T: type, data: T, cloned: *T, allocator: ?Allocator) !void { |
There was a problem hiding this comment.
it's kind of weird, in terms of interface, that you need both an allocator and a pre-allocated output. I'd say: either allocate nothing or all of it.
| pub fn sszClone(self: *const Self, cloned: *Self, allocator: ?Allocator) !void { | ||
| const alloc = allocator orelse return error.AllocatorRequired; |
There was a problem hiding this comment.
ideally, the interface should be changed to:
| pub fn sszClone(self: *const Self, cloned: *Self, allocator: ?Allocator) !void { | |
| const alloc = allocator orelse return error.AllocatorRequired; | |
| pub fn sszClone(self: *const Self, cloned: *Self, allocator: Allocator) !void { |
Because we always have an allocation. In the case of objects that don't need an allocation, it's fine to pass a dummy allocator, or just not use the parameter. It's ok, if it makes the whole code easier to implement, then I won't die on this hill.
| } | ||
|
|
||
| /// Clones this bitlist's backing storage. | ||
| pub fn sszClone(self: *const Self, cloned: *Self, allocator: ?Allocator) !void { |
There was a problem hiding this comment.
as mentioned in the general review comment, this should simply be called clone
|
Forgot to add: the code looks good to me, but most of it shouldn't be in the ssz lib |
3 participants
Adds
clone(T, data, *T, ?Allocator)as a direct replacement for theserialize → deserializeround-trip previously used to deep-copy SSZ values. It produces an independent copy without any intermediate buffer, avoiding the encode/decode cost.