-
Notifications
You must be signed in to change notification settings - Fork 7
box_tree: make NodeId tuple fields private, use named methods to access
#40
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
base: main
Are you sure you want to change the base?
Conversation
waywardmonkeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be better as a struct-struct with 2 fields rather than a tuple struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be better as a struct-struct with 2 fields rather than a tuple struct?
Yes, that makes sense. There may also be value in thinking about the memory layout a bit, to allow methods like id_is_newer to be compiled down to a single u64 comparison without requiring any shifting of memory (this type is a likely target for exposing through FFI in the future, I think, so it'll get repr(C) anyway). In Vello sparse we do some cfg based on endianness, but potentially here we can decide to just do the optimization assuming the target is little endian.
bcadd80 to
bcb76ac
Compare
tomcur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've given the struct named fields, but haven't made them pub (yet?) for two reasons: first, the idx() method casts to usize, which may be nice to keep for ergonomics, regardless of the exact internal representation. Second, we may still want to have a deterministic layout / packing of this struct, but perhaps we need to think more about the "ID is newer" semantics: that's currently based on generation first, which is perhaps fine for just having a deterministic ordering, but generations are only meaningfully comparable if the node has the same idx (as generation tends to be tracked per-idx).
|
Overall, this is the right thing ... but feel like maybe we need to think about some of these details a little. |
No description provided.