-
Notifications
You must be signed in to change notification settings - Fork 0
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
some comments on list #1
Comments
re: upstream functions, I can issue a PR to the nursery for those first if you'd like, otherwise the main ZZ repo.
Great to know. This was definitely an exercise in getting to know theories and how hey work
Got it
Hmm yeah that makes sense. I'd like to think on that one a bit. Naming things is hard
Yes that is correct. I do not see an obvious way to get around that. Do you have any suggestions/tips? I'll definitely be looking out
I have been running into naming issues with methods on structs such as
Good to know!
You're correct. It is not needed. The
It is not needed and left over when heap allocations used. I'll remove them destroy functions
ah yeah that should be |
@aep just pushed some changes that address the implementation and usage in your comments |
unfortunately there's no inline review option unless you open a PR, so i'll comment this way
zx/modules/utils/src/math.zz
Line 1 in c3c2625
these should really be upstreamed
https://github.com/zx-project/zx/blob/master/modules/list/src/node.zz#L79
this exists
https://github.com/zetzit/zz/blob/master/modules/std/src/lib.zz#L10
but memory::zero sounds better than std::memset. should probably rename upstream
https://github.com/zx-project/zx/blob/master/modules/list/src/list.zz#L36
this probably meaningless. theories are very expensive (slow compilation) so don't use them excessively.
https://github.com/zx-project/zx/blob/master/modules/list/src/list.zz#L54
i like this struct, but for clarity head and tail should be marked unsafe, because they might be null.
https://github.com/zx-project/zx/blob/master/modules/list/src/list.zz#L115
(opinion) i think default() makes sense as function name here because you're filling in default values for the callbacks
https://github.com/zx-project/zx/blob/master/modules/list/src/list.zz#L106
does that mean this list can only store borrowed pointers? i mean, thats's actually a good idea.
i'm just scared of it because lifetimes aren't well defined in zetz yet. you could store a pointer to something that later gets deleted. anyway, that's an issue i need to work on, just be prepared for breaking changes in that direction :)
https://github.com/zx-project/zx/blob/master/modules/list/src/list.zz#L89
does that really have to be in a separate module? i'd rather have clean single file thingies with as little code as possible (more code = harder to read & more bugs)
https://github.com/zx-project/zx/blob/master/modules/list/src/iterator.zz#L58
dont return self pointers. that's confusing and currently makes both the prover explode as well as the nodejs export do very wrong things.
sorry that these things aren't useful compile errors. things are still moving around alot :)
https://github.com/zx-project/zx/blob/master/modules/list/src/iterator.zz#L140
i dont understand why this exists. why not just break?
https://github.com/zx-project/zx/blob/master/modules/list/src/iterator.zz#L174
destructors should be called drop(). like C++, drop() functions are called automatically when the lifetime ends (which is currently really broken, bear with me)
i'm not entirely sure what this does tho. generally structures that need no destructor are easier to manage
https://github.com/zx-project/zx/blob/master/modules/list/src/node.zz#L87
what does this do? deref usually refers to looking at a value refferenced by a pointer.
does the function remove a node from a list?
The text was updated successfully, but these errors were encountered: