jwerle
commented
4 years ago re: upstream functions, I can issue a PR to the nursery for those first if you'd like, otherwise the main ZZ repo.
/modules/list/src/list.zz@master#L36 this probably meaningless. theories are very expensive (slow compilation) so don't use them excessively.
Great to know. This was definitely an exercise in getting to know theories and how hey work
/modules/list/src/list.zz@master#L54 i like this struct, but for clarity head and tail should be marked unsafe, because they might be null.
Got it
/modules/list/src/list.zz@master#L115 (opinion) i think default() makes sense as function name here because you're filling in default values for the callbacks
Hmm yeah that makes sense. I'd like to think on that one a bit. Naming things is hard
/modules/list/src/list.zz@master#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 :)
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
/modules/list/src/list.zz@master#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)
I have been running into naming issues with methods on structs such as make() or init() and opted for modularity by using multiple files
/modules/list/src/iterator.zz@master#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 :)
Good to know!
/modules/list/src/iterator.zz@master#L140 i dont understand why this exists. why not just break?
You're correct. It is not needed. The end() method exists for API completion and that example is very contrived
/modules/list/src/iterator.zz@master#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
It is not needed and left over when heap allocations used. I'll remove them destroy functions
/modules/list/src/node.zz@master#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?
ah yeah that should be unref() and yes that is what it does. I've already corrected the name, just need to push it up
unfortunately there's no inline review option unless you open a PR, so i'll comment this way
https://github.com/zx-project/zx/blob/c3c2625ef1e7d0ddac0904750e3ef9eac919384b/modules/utils/src/math.zz#L1
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?