Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| /** | ||
| * @brief Returns the size of the tree | ||
| */ | ||
| size_t size() const { return num_bits_; } |
There was a problem hiding this comment.
WARNING: size() returns bit-length, not node count
size() is documented as the tree size but returns num_bits_ (2 * number of nodes). Callers expecting node count will get a 2x value. Consider returning num_bits_ / 2 or renaming to clarify the unit.
| return 0; | ||
| } | ||
| BpNode child = first_child(node); | ||
| int child_count = 1; |
There was a problem hiding this comment.
WARNING: child_count uses int and can overflow
For large degrees, int can overflow before being returned as size_t, which can wrap to a huge value. Use size_t for the counter to avoid overflow.
| const std::vector<std::vector<size_t>>& adj) { | ||
| size_t bp_size = tree_size * 2; | ||
| std::vector<uint64_t> bp((bp_size + 63) / 64, 0); | ||
| std::vector<std::pair<int, int>> stack; |
There was a problem hiding this comment.
WARNING: pos and stack indices use int, risk overflow
pos indexes into the BP bitvector and can exceed INT_MAX for large trees, leading to out-of-bounds writes. Use size_t for pos and the stack pair to keep indices in range.
| std::string result; | ||
| result.resize(num_bits); | ||
|
|
||
| for (size_t i = 0; i < num_bits; ++i) { |
There was a problem hiding this comment.
SUGGESTION: Optimize bit-to-string conversion for large vectors
The per-bit loop does a shift and index for every bit. A byte-level LUT (256-entry) or word-level conversion can reduce overhead when exporting large bitvectors.
There was a problem hiding this comment.
This will wait for better times
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (2 files)
Reviewed by gpt-5.2-codex-20260114 · 147,189 tokens |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
- Coverage 86.35% 81.64% -4.72%
==========================================
Files 11 13 +2
Lines 2850 3012 +162
Branches 562 590 +28
==========================================
- Hits 2461 2459 -2
- Misses 254 419 +165
+ Partials 135 134 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| build(leaf_block_bits, max_overhead); | ||
| } | ||
| public: |
There was a problem hiding this comment.
Please make it so that there's a single private section on top and a single public section at bottom of class declaration
| /** | ||
| * @brief A node class of BP tree | ||
| */ | ||
| struct BpNode { |
There was a problem hiding this comment.
Please make it so it is public part of the BpTree class and is simply called Node.
| * @brief A tree class based on the balances parentheses (BP) | ||
| * representation | ||
| */ | ||
| class BpTree { |
There was a problem hiding this comment.
I also think it is better to write abbreviations in upper case for class names, i.e. BPTree
| class BpTree { | ||
| private: | ||
| const size_t num_bits_; | ||
| RmMTree rmm; |
| /** | ||
| * @brief Indicates if @p node is a root | ||
| */ | ||
| static bool is_root(const BpNode& node) { return node.number == 0; } |
There was a problem hiding this comment.
Although the implementation doesn't depend on the tree instance, in fact it is a tree method, so please remove static.
| /** | ||
| * @brief Returns first child of a @p node | ||
| */ | ||
| static BpNode first_child(const BpNode& node) { |
| gtest_main | ||
| ${PIXIE_DIAGNOSTICS_LIBS}) | ||
|
|
||
| add_executable(bp_tree_tests |
There was a problem hiding this comment.
Tests should be added to coverage/build-test workflows (but not for AVX-512).
2 participants
No description provided.