Align graph wrappers with tree API#198
Merged
renau merged 2 commits intomasc-ucsc:mainfrom Mar 20, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe pull request refactors handle validation in the graph library by removing graph pointers from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use Clang for C/C++ static analysis and code quality checks.Clang provides comprehensive static analysis for C and C++ code, including syntax checking, type checking, and various warning diagnostics. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hhds/graph.cpp`:
- Around line 711-724: Graph::assert_node_exists and Graph::assert_pin_exists
currently only validate local indices (via Node_class::get_raw_nid and
Pin_class::get_pin_pid) so a handle from a different live Graph with the same
local id will pass and index the wrong entry; modify the handle representation
to carry a per-graph owner discriminator (e.g. a generation/serial field) and
update Graph::assert_node_exists and Graph::assert_pin_exists to validate that
the handle's owner discriminator matches this graph's current discriminator
before doing the index/bounds checks against node_table and pin_table; update
any constructors/factories that produce Node_class/Pin_class and adjust
downstream tests (hhds/tests/iterators_impl.cpp lines ~929-949) to create/expect
handles with the correct owner discriminator.
- Around line 1095-1100: The create_pin path currently strips only the direction
bit and will misinterpret pin-encoded ids as node handles; add a guard at the
start of Graph::create_pin to assert or reject that the incoming nid is a node
handle (i.e. its low two bits are zero) before masking/shifting. Concretely,
before `nid &= ~static_cast<Nid>(2);` add a check like `assert((nid & 3) == 0 &&
"create_pin: expected node handle, got pin-encoded id");` (or return/throw an
error) so passing an existing pin Pid cannot be silently reinterpreted as a
different node index. Ensure the check uses the same Nid/Pid types and message
references `Graph::create_pin`, `nid`, and existing masking/shift logic.
In `@hhds/graph.hpp`:
- Around line 399-404: The current is_valid overloads only test non-zero raw
values and thus accept malformed encodings (e.g., Node_class(2) or
Pin_class(0,0,1)); update all is_valid overloads (is_valid for Node_class,
Pin_class, Node_flat, Pin_flat, Node_hier, Pin_hier) to mirror the structural
bit/index validation used by assert_node_exists/assert_pin_exists: decode the
handle components and verify the tag/bitmap/slot/index fields match the
non-sentinel, in-range values (i.e., ensure the decoded index/slot is not the
sentinel and any tag/bit flags are correct) rather than just testing raw != 0 so
malformed encodings are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b435d56-21e7-4f84-aae2-789903724800
📒 Files selected for processing (4)
hhds/graph.cpphhds/graph.hpphhds/tests/iterators.cpphhds/tests/iterators_impl.cpp
Comment on lines
+711
to
+724
| void Graph::assert_node_exists(const Node_class& node) const noexcept { | ||
| assert_accessible(); | ||
| if (node.graph != nullptr) { | ||
| node.graph->assert_accessible(); | ||
| assert(node.graph == this && "node handle belongs to a different graph"); | ||
| } | ||
| const Nid raw_nid = node.get_raw_nid(); | ||
| const Nid actual_id = raw_nid >> 2; | ||
| assert((raw_nid & static_cast<Nid>(1)) == 0 && "node handle is not a node"); | ||
| assert(actual_id > 0 && actual_id < node_table.size() && "node handle is invalid for this graph"); | ||
| } | ||
|
|
||
| void Graph::assert_compatible(const Pin_class& pin) const noexcept { | ||
| void Graph::assert_pin_exists(const Pin_class& pin) const noexcept { | ||
| assert_accessible(); | ||
| if (pin.graph != nullptr) { | ||
| pin.graph->assert_accessible(); | ||
| assert(pin.graph == this && "pin handle belongs to a different graph"); | ||
| } | ||
| const Pid raw_pid = pin.get_pin_pid(); | ||
| const Pid actual_id = raw_pid >> 2; | ||
| assert((raw_pid & static_cast<Pid>(1)) == static_cast<Pid>(1) && "pin handle is not a pin"); | ||
| assert(actual_id > 0 && actual_id < pin_table.size() && "pin handle is invalid for this graph"); |
Contributor
There was a problem hiding this comment.
These checks no longer reject foreign class handles.
Node_class and Pin_class now carry only graph-local ids. A handle from another live Graph with the same local index will pass these bounds checks and hit this graph's node_table / pin_table entry instead, so mixed-graph calls can silently read or mutate the wrong object. These wrappers still need an owner discriminator (for example a per-graph serial/generation) if graph APIs are going to keep accepting class handles. Downstream, hhds/tests/iterators_impl.cpp:929-949 will also need to change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hhds/graph.cpp` around lines 711 - 724, Graph::assert_node_exists and
Graph::assert_pin_exists currently only validate local indices (via
Node_class::get_raw_nid and Pin_class::get_pin_pid) so a handle from a different
live Graph with the same local id will pass and index the wrong entry; modify
the handle representation to carry a per-graph owner discriminator (e.g. a
generation/serial field) and update Graph::assert_node_exists and
Graph::assert_pin_exists to validate that the handle's owner discriminator
matches this graph's current discriminator before doing the index/bounds checks
against node_table and pin_table; update any constructors/factories that produce
Node_class/Pin_class and adjust downstream tests (hhds/tests/iterators_impl.cpp
lines ~929-949) to create/expect handles with the correct owner discriminator.
Comment on lines
1095
to
1100
| auto Graph::create_pin(Nid nid, Port_id pid) -> Pid { | ||
| assert_accessible(); | ||
| // nid is << 2 here but port_id is not << 2 id (here but pin id in actual) is also not << 2 | ||
| nid &= ~static_cast<Nid>(2); // Pin ownership is by node identity, independent of edge role bit. | ||
| const Nid actual_nid = nid >> 2; | ||
| assert(actual_nid > 0 && actual_nid < node_table.size() && "create_pin: node handle is invalid for this graph"); | ||
| Pid id = pin_table.size(); |
Contributor
There was a problem hiding this comment.
Reject pin-encoded ids in the raw create_pin path.
This overload only strips the direction bit. Passing an odd/pin-encoded value will reinterpret its numeric payload as a node index, so create_pin(existing_pin_pid, ...) silently attaches the new pin to whichever node lives at existing_pin_pid >> 2.
Suggested guard
auto Graph::create_pin(Nid nid, Port_id pid) -> Pid {
assert_accessible();
+ assert((nid & static_cast<Nid>(1)) == 0 && "create_pin expects a node handle, not a pin id");
nid &= ~static_cast<Nid>(2); // Pin ownership is by node identity, independent of edge role bit.
const Nid actual_nid = nid >> 2;
assert(actual_nid > 0 && actual_nid < node_table.size() && "create_pin: node handle is invalid for this graph");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hhds/graph.cpp` around lines 1095 - 1100, The create_pin path currently
strips only the direction bit and will misinterpret pin-encoded ids as node
handles; add a guard at the start of Graph::create_pin to assert or reject that
the incoming nid is a node handle (i.e. its low two bits are zero) before
masking/shifting. Concretely, before `nid &= ~static_cast<Nid>(2);` add a check
like `assert((nid & 3) == 0 && "create_pin: expected node handle, got
pin-encoded id");` (or return/throw an error) so passing an existing pin Pid
cannot be silently reinterpreted as a different node index. Ensure the check
uses the same Nid/Pid types and message references `Graph::create_pin`, `nid`,
and existing masking/shift logic.
Comment on lines
+399
to
+404
| [[nodiscard]] static constexpr bool is_valid(Node_class node) noexcept { return node.get_raw_nid() != 0; } | ||
| [[nodiscard]] static constexpr bool is_valid(Pin_class pin) noexcept { return pin.get_pin_pid() != 0; } | ||
| [[nodiscard]] static constexpr bool is_valid(Node_flat node) noexcept { return node.get_raw_nid() != 0; } | ||
| [[nodiscard]] static constexpr bool is_valid(Pin_flat pin) noexcept { return pin.get_pin_pid() != 0; } | ||
| [[nodiscard]] static constexpr bool is_valid(const Node_hier& node) noexcept { return node.get_raw_nid() != 0; } | ||
| [[nodiscard]] static constexpr bool is_valid(const Pin_hier& pin) noexcept { return pin.get_pin_pid() != 0; } |
Contributor
There was a problem hiding this comment.
is_valid(...) currently accepts malformed handle encodings.
A pure non-zero check makes values like Node_class(2) or Pin_class(0, 0, 1) report valid even though their decoded index is still the sentinel slot. These helpers should at least mirror the structural bit/index checks already used by assert_node_exists / assert_pin_exists, and the same tightening should be applied to the flat/hier variants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hhds/graph.hpp` around lines 399 - 404, The current is_valid overloads only
test non-zero raw values and thus accept malformed encodings (e.g.,
Node_class(2) or Pin_class(0,0,1)); update all is_valid overloads (is_valid for
Node_class, Pin_class, Node_flat, Pin_flat, Node_hier, Pin_hier) to mirror the
structural bit/index validation used by assert_node_exists/assert_pin_exists:
decode the handle components and verify the tag/bitmap/slot/index fields match
the non-sentinel, in-range values (i.e., ensure the decoded index/slot is not
the sentinel and any tag/bit flags are correct) rather than just testing raw !=
0 so malformed encodings are rejected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR advances the graph-side API cleanup from
api_todo.mdby making graph wrappers more consistent with the tree wrapper model.Changes
Node_classandPin_classinto value-style wrappers without storedGraph*node.create_pin(...)to container-ownedgraph.create_pin(node, ...)is_valid(...)helpers for:Node_classPin_classNode_flatPin_flatNode_hierPin_hierWhy
Testing
bazel test //hhds:iterators_impl //hhds:graph_testbazel test --test_tag_filters=manual //hhds:iteratorsbazel test //hhds:tree_wrapper_test //hhds:tree_type_test //hhds:forest_correctnessSummary by CodeRabbit
New Features
Graph::is_valid()validation methods for handle types.API Changes
Graph::create_pin(node, port_id)instead ofnode.create_pin(port_id).Bug Fixes