fix: properly handle basic type vector/list in TreeView #80
Conversation
Summary of ChangesHello @ensi321, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the handling of basic type vectors and lists within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly refactors the handling of basic type vectors and lists in TreeView by introducing helper functions to calculate chunk and element offsets, and fixes the packing logic for BoolType. The overall approach is sound. I've found one critical typo that would prevent compilation and a couple of minor suggestions to simplify the code in bool.zig. While not part of this PR's changes, please also note that UintType.tree.fromValuePacked appears to have a bug in its offset calculation that is surfaced by the changes in tree_view.zig and should be fixed in a follow-up.
| if (ST.kind != .vector and ST.kind != .list) { | ||
| @compileError("chunkIndex is only available for vector or list types"); | ||
| } | ||
| return if (comptime isBasicType(ST.element)) index / elementsPerChunk() else index; |
There was a problem hiding this comment.
There seems to be a typo here. ST.element should be ST.Element to match the field name in the SSZ type definitions and for consistency with the elementOffset function. This will cause a compilation error.
return if (comptime isBasicType(ST.Element)) index / elementsPerChunk() else index;
| const offset = index % 256; | ||
| const byte_index = offset / 8; | ||
| const bit_index = offset % 8; |
There was a problem hiding this comment.
The index parameter passed to this function is the result of elementOffset, which is already calculated as index % 256. Therefore, const offset = index % 256; is redundant. You can remove the offset variable and directly use index to calculate byte_index and bit_index for simplification.
const byte_index = index / 8;
const bit_index = index % 8;
| const offset = index % 256; | ||
| const byte_index = offset / 8; | ||
| const bit_index = offset % 8; |
There was a problem hiding this comment.
Similar to toValuePacked, the index parameter here is already the offset within the chunk (< 256), so calculating index % 256 is redundant. You can simplify the code by removing the offset variable.
const byte_index = index / 8;
const bit_index = index % 8;
|
@ensi321 Can I take a look at this if you are busy on other work? |
toValuePackedwhen getting basic typeBoolType().tree