Exclude built-in nodes from forward_class and align traversal tests#194
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new constant Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
Pull request overview
This PR changes Graph::forward_class() to return only user-created nodes (excluding built-in INVALID, INPUT, OUTPUT, and CONST) and updates iterator tests to match the new contract.
Changes:
- Updated
rebuild_forward_class_cache()traversal to start from the first user node index and ignore dependencies to built-in nodes. - Updated
forward_class()cache-size expectations to reflect user nodes only. - Adjusted
LazyTraversal.ForwardClassTopologicalOrdertest expectation from 7 nodes to 4 nodes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
hhds/graph.cpp |
Excludes built-in nodes from forward topological cache construction and adjusts expected cache sizing accordingly. |
hhds/tests/iterators_impl.cpp |
Updates the forward traversal test to expect only user-created nodes in the returned span. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| constexpr size_t first_user_node_idx = 4; // 0:invalid, 1:INPUT, 2:OUTPUT, 3:CONST | ||
| forward_class_cache_.clear(); | ||
| if (node_table.size() <= 1) { | ||
| if (node_table.size() <= first_user_node_idx) { | ||
| forward_class_cache_valid_ = true; |
There was a problem hiding this comment.
first_user_node_idx is a magic number that is now duplicated here and again in Graph::forward_class(). To avoid the two code paths drifting (and causing cache-size mismatches / repeated rebuilds if one is updated without the other), consider defining a single shared constant (e.g., a static constexpr on Graph or a file-scope constant) and using it in both places.
| constexpr size_t first_user_node_idx = 4; // 0:invalid, 1:INPUT, 2:OUTPUT, 3:CONST | ||
| const size_t expected_size = node_table.size() > first_user_node_idx ? node_table.size() - first_user_node_idx : 0; | ||
| if (!forward_class_cache_valid_ || forward_class_cache_.size() != expected_size) { |
There was a problem hiding this comment.
first_user_node_idx is re-declared here even though rebuild_forward_class_cache() also declares it. Centralizing this constant (ideally near the built-in node definitions) would reduce the risk of future inconsistencies in expected cache size vs. cache contents.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hhds/graph.cpp (1)
773-775: Avoid duplicating the reserved-node boundary constant.
first_user_node_idxis repeated in multiple methods; centralizing it reduces drift risk if reserved-node layout changes.♻️ Suggested refactor
+namespace { +constexpr size_t kFirstUserNodeIdx = 4; // 0:invalid, 1:INPUT, 2:OUTPUT, 3:CONST +} + void Graph::rebuild_forward_class_cache() const { - constexpr size_t first_user_node_idx = 4; // 0:invalid, 1:INPUT, 2:OUTPUT, 3:CONST forward_class_cache_.clear(); - if (node_table.size() <= first_user_node_idx) { + if (node_table.size() <= kFirstUserNodeIdx) { forward_class_cache_valid_ = true; return; } ... - for (size_t driver_idx = first_user_node_idx; driver_idx < node_count; ++driver_idx) { + for (size_t driver_idx = kFirstUserNodeIdx; driver_idx < node_count; ++driver_idx) { ... - forward_class_cache_.reserve(node_count - first_user_node_idx); + forward_class_cache_.reserve(node_count - kFirstUserNodeIdx); } auto Graph::forward_class() const -> std::span<const Node_class> { - constexpr size_t first_user_node_idx = 4; // 0:invalid, 1:INPUT, 2:OUTPUT, 3:CONST - const size_t expected_size = node_table.size() > first_user_node_idx ? node_table.size() - first_user_node_idx : 0; + const size_t expected_size = node_table.size() > kFirstUserNodeIdx ? node_table.size() - kFirstUserNodeIdx : 0;Also applies to: 1103-1104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hhds/graph.cpp` around lines 773 - 775, The code duplicates the reserved-node boundary constant `first_user_node_idx`; define a single constant (e.g., a static constexpr or enum value such as `kFirstUserNodeIdx`) on the Graph class or in the relevant header and replace all local `first_user_node_idx` occurrences (including the usage alongside `forward_class_cache_` and checks against `node_table.size()` at the shown site and around lines 1103-1104) to reference that centralized symbol so the reserved-node layout is maintained in one place.hhds/tests/iterators_impl.cpp (1)
554-568: Harden the ordering assertions to avoid silent map key insertion.Using
pos[...]can hide missing-node bugs via default insertion. Preferfind/atwith explicit presence asserts before ordering checks.✅ Suggested test hardening
const hhds::Nid n1_id = n1.get_raw_nid() & ~static_cast<hhds::Nid>(3); const hhds::Nid n2_id = n2.get_raw_nid() & ~static_cast<hhds::Nid>(3); const hhds::Nid n3_id = n3.get_raw_nid() & ~static_cast<hhds::Nid>(3); const hhds::Nid n4_id = n4.get_raw_nid() & ~static_cast<hhds::Nid>(3); - EXPECT_LT(pos[n1_id], pos[n2_id]); - EXPECT_LT(pos[n1_id], pos[n3_id]); - EXPECT_LT(pos[n2_id], pos[n4_id]); - EXPECT_LT(pos[n3_id], pos[n4_id]); + const auto it1 = pos.find(n1_id); + const auto it2 = pos.find(n2_id); + const auto it3 = pos.find(n3_id); + const auto it4 = pos.find(n4_id); + ASSERT_NE(it1, pos.end()); + ASSERT_NE(it2, pos.end()); + ASSERT_NE(it3, pos.end()); + ASSERT_NE(it4, pos.end()); + + EXPECT_LT(it1->second, it2->second); + EXPECT_LT(it1->second, it3->second); + EXPECT_LT(it2->second, it4->second); + EXPECT_LT(it3->second, it4->second);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hhds/tests/iterators_impl.cpp` around lines 554 - 568, The test currently uses pos[...] which silently inserts default entries on missing keys; update the test to assert presence and avoid operator[]: when building pos keep using pos.emplace/order loop, then before each EXPECT_LT call check presence with pos.find(key) != pos.end() or ASSERT_TRUE(pos.contains(key)) (using at least one presence assert for n1_id,n2_id,n3_id,n4_id), and replace pos[...] in the comparisons with pos.at(n1_id)/pos.at(n2_id)/pos.at(n3_id)/pos.at(n4_id) (or use the iter->second after find) to ensure missing keys fail the test instead of creating default values; reference variables: order, pos, hhds::Nid, n1.get_raw_nid(), n2.get_raw_nid(), n3.get_raw_nid(), n4.get_raw_nid().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hhds/graph.cpp`:
- Around line 773-775: The code duplicates the reserved-node boundary constant
`first_user_node_idx`; define a single constant (e.g., a static constexpr or
enum value such as `kFirstUserNodeIdx`) on the Graph class or in the relevant
header and replace all local `first_user_node_idx` occurrences (including the
usage alongside `forward_class_cache_` and checks against `node_table.size()` at
the shown site and around lines 1103-1104) to reference that centralized symbol
so the reserved-node layout is maintained in one place.
In `@hhds/tests/iterators_impl.cpp`:
- Around line 554-568: The test currently uses pos[...] which silently inserts
default entries on missing keys; update the test to assert presence and avoid
operator[]: when building pos keep using pos.emplace/order loop, then before
each EXPECT_LT call check presence with pos.find(key) != pos.end() or
ASSERT_TRUE(pos.contains(key)) (using at least one presence assert for
n1_id,n2_id,n3_id,n4_id), and replace pos[...] in the comparisons with
pos.at(n1_id)/pos.at(n2_id)/pos.at(n3_id)/pos.at(n4_id) (or use the iter->second
after find) to ensure missing keys fail the test instead of creating default
values; reference variables: order, pos, hhds::Nid, n1.get_raw_nid(),
n2.get_raw_nid(), n3.get_raw_nid(), n4.get_raw_nid().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec542a8e-4a18-4bf6-afae-dfb7768470d6
📒 Files selected for processing (2)
hhds/graph.cpphhds/tests/iterators_impl.cpp
Summary
This PR updates
forward_class()to return only user-created nodes (exclude built-inINPUT,OUTPUT, andCONSTnodes), and aligns iterator tests with that contract.Why
forward_classbehavior was inconsistent with test intent.One test expected only user nodes in topological order (
28, 24, 16, 20), while another expected built-ins to be included (size == 7).Changes
hhds/graph.cppto:LazyTraversal.ForwardClassTopologicalOrderinhhds/tests/iterators_impl.cpp:ASSERT_EQ(order.size(), 7)→ASSERT_EQ(order.size(), 4)Behavior Change
Before:
forward_class()returned built-ins + user nodes.After:
forward_class()returns user nodes only.Example with 4 user nodes:
28, 24, 16, 20)Validation
bazel test -c dbg //hhds:iterators_impl --test_output=errorsbazel test -c dbg //hhds:graph_test //hhds:iterators_impl --test_output=errorsSummary by CodeRabbit
Release Notes