Conversation
Add Gen_Segment_Tree, Gen_Lazy_Segment_Tree (with policies), and Segment_Tree_Beats for dynamic range queries. Includes comprehensive tests and documentation updates. Remove old test scripts.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a comprehensive segment-tree header ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb89f51dbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds a new segment tree module to Aleph-w, covering classic point-update/range-query segment trees, lazy-propagation segment trees, and Segment Tree Beats, along with tests and README documentation to position segment trees among existing range-query structures.
Changes:
- Introduces
tpl_segment_tree.HwithGen_Segment_Tree,Gen_Lazy_Segment_Tree(policy-based), andSegment_Tree_Beats. - Adds comprehensive GTest coverage for segment trees and (separately) sparse tables.
- Updates build configuration and README to expose the new header and document usage (and references a new example target).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
tpl_segment_tree.H |
New header implementing three segment tree variants with Doxygen docs. |
Tests/tpl_segment_tree_test.cc |
New GTest suite covering point updates, lazy updates, and Segment Tree Beats. |
Tests/tpl_sparse_table_test.cc |
New GTest suite expanding coverage for sparse tables. |
Tests/depend.sh |
Removed legacy dependency script. |
Tests/clean |
Removed legacy clean script. |
Tests/CMakeLists.txt |
Registers tpl_segment_tree_test in the explicit test list. |
README.md |
Adds segment tree documentation and API table entries. |
Examples/CMakeLists.txt |
Adds segment_tree_example target name (source file not present in this PR). |
CMakeLists.txt |
Adds tpl_segment_tree.H to the header list. |
The `tpl_segment_tree.H` file has been refactored to improve Doxygen documentation for `Gen_Segment_Tree`, `Gen_Lazy_Segment_Tree`, and `Segment_Tree_Beats`. The commit also includes clarifications for `SegmentTreeOp` and new concepts like `SegmentTreePolicy` for lazy segment trees, making the API clearer and more robust. The `query` method in `Gen_Lazy_Segment_Tree` is now `const` due to mutable lazy state, allowing safe calls via `const` references.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tpl_segment_tree.H`:
- Around line 1238-1239: Segment_Tree_Beats is missing a DynList<T> constructor
(unlike Gen_Segment_Tree/Gen_Lazy_Segment_Tree); add a constructor
Segment_Tree_Beats(const DynList<T>& values) that initializes tree and n (use
tree(values.size()==0?1:4*values.size()) and n = values.size()), calls
init_all_nodes(), and when n>0 copies values into a temporary Array<T>
(Array<T>::create(n)) using values.get_it() before calling fill_and_build with a
lambda that returns tmp(j); ensure you reference the existing members tree, n,
init_all_nodes(), fill_and_build(), and Array<T>::create to match other
constructors' behavior and placement (after the std::vector constructor).
🧹 Nitpick comments (6)
tpl_segment_tree.H (6)
216-255:fill_and_buildinitializes entire tree then overwrites leaves, then rebuilds — redundant O(n) pass.
fill_and_buildfirst initializes all4*nnodes toidentity(line 220–221), then callsfill_leaves_recursiveto set leaf values, then callsbuildto recompute internal nodes. The initial full-array identity fill is unnecessary becausefill_leaves_recursivesets every leaf andbuildrecomputes every internal node from its children.Removing the loop on lines 220–221 would save one O(n) pass over the backing array. The same pattern appears in
Gen_Lazy_Segment_Tree::fill_and_build(which already omits it), so the two variants are inconsistent.♻️ Remove redundant initialization loop
template <class Getter> void fill_and_build(Getter getter) { - // Initialize entire tree with identity - for (size_t i = 0; i < tree.size(); ++i) - tree(i) = identity; - // Place values in leaves via recursive descent fill_leaves_recursive(1, 0, n - 1, getter); // Build internal nodes bottom-up build(1, 0, n - 1); }
430-436:values()is O(n log n) but could be O(n) by traversing leaves directly.Each
get(i)call does a full root-to-leaf query. SinceGen_Segment_Treehas no lazy propagation, you can traverse the implicit tree once to collect allnleaf values in O(n). The same applies to all three tree variants (though the lazy ones need push-down during traversal). Documented complexity is accurate, so this is just an optimization opportunity.
620-637: Clean policy concept — verifystd::pair<bool, T>satisfiesequality_comparableforAssign_Sum_Policy.
SegmentTreePolicyrequiresstd::equality_comparable<lazy_type>. ForAssign_Sum_Policy<T>,lazy_type = std::pair<bool, T>, which is equality-comparable only ifTis. This works for arithmetic types but the constraint isn't documented onAssign_Sum_Policy.
1261-1310:pull_upisconstbut mutatestree(node)— correct only becausetreeismutable.This works, but the
constqualifier onpull_up(andpush_chmin_to_child,push_chmax_to_child) is semantically misleading since these functions modify observable node state (sum, max_val, etc.), not just cached/lazy metadata. Consider documenting why these areconst(themutablerationale), as per coding guidelines.
1312-1332:push_chmin_to_childis also called on the current node (not a child) inchmin_implline 1395.The function name suggests it only operates on children, but it is also called with
nodeitself in the "tag condition" path ofchmin_impl. The logic is correct — the function applies a chmin tag to any node regardless of whether it's a child — but the name is misleading. Same forpush_chmax_to_childcalled at line 1414.Consider renaming to
apply_chmin/apply_chmaxor adding a brief comment at the call sites.
146-153: Both tree classes usestd::vector-style internal storage — consider documenting thread-safety.Per coding guidelines, thread-safety guarantees should be documented. These segment trees are not thread-safe (concurrent reads on
Gen_Lazy_Segment_TreeandSegment_Tree_Beatsmutatemutablestate viapush_down). A brief@note Not thread-safein the class Doxygen would be helpful.Also applies to: 844-857
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tpl_segment_tree.H`:
- Around line 1561-1564: Segment_Tree_Beats declares its move constructor and
move-assignment as unconditionally noexcept which can mismatch when Array<Node>
move can throw; change both declarations to conditional noexcept like the other
classes (Gen_Segment_Tree / Gen_Lazy_Segment_Tree) so they mirror the move
safety of Array<Node>. Specifically, update the Segment_Tree_Beats move
constructor and operator= to use noexcept(...) that checks Array<Node>'s nothrow
move (e.g. noexcept(std::is_nothrow_move_constructible_v<Array<Node>>) or the
same noexcept(...) expression used in Gen_Segment_Tree/Gen_Lazy_Segment_Tree)
and keep them = default.
🧹 Nitpick comments (7)
tpl_segment_tree.H (7)
146-165:buildis called afterfill_leaves_recursiveredundantly visits leaves.
buildrecurses to every leaf (start == end) only to return immediately. This is O(n) extra function calls with no work at the leaves. A simple iterative bottom-up loop fromndown to1on the internal nodes (indices[1, n)in classic 1-based layout) would halve the recursion. Not a correctness issue and the current approach is canonical, so this is purely a performance nit.
216-243:fill_and_buildinitialises the entire tree toidentity, then fills leaves, then rebuilds internal nodes — three passes over the array.The first pass (lines 220–221) writes
identityto all4*nslots. The second pass (fill_leaves_recursive) overwrites the leaf positions. The third pass (build) recomputes internal nodes. The first pass could be eliminated ifbuildis relied upon to set internal nodes (which it does), and leaves are guaranteed set byfill_leaves_recursive. The only risk is uninitialised internal nodes in thetreearray, butArray<T>is already value-initialised in the constructor (Array(size, id)), so the explicit loop is redundant for the size-based constructor and the other constructors that passidas the fill value.
430-436:values()is O(n log n) — could be O(n) for the non-lazy tree.Since
Gen_Segment_Treehas no lazy propagation, leaf values can be read directly from the backing array by walking the implicit tree to each leaf in a single O(n) traversal, similar tofill_leaves_recursivebut reading instead of writing. The current implementation callsget(i)(which isquery(i,i)= O(log n)) for each element.♻️ Sketch: O(n) values extraction
Array<T> values() const { auto ret = Array<T>::create(n); extract_leaves(1, 0, n - 1, ret); return ret; } private: void extract_leaves(size_t node, size_t start, size_t end, Array<T> & out) const { if (start == end) { out(start) = tree(node); return; } const size_t mid = start + (end - start) / 2; extract_leaves(2 * node, start, mid, out); extract_leaves(2 * node + 1, mid + 1, end, out); }
663-666: Potential integer overflow inAdd_Sum_Policy::apply.
delta * static_cast<T>(count)can overflow whenTis a narrow type likeintandcountis large. The same concern applies toAssign_Sum_Policy::apply(line 800) andSegment_Tree_Beats::push_chmin_to_child(line 1320). This is inherent to the algorithm's arithmetic, but consider documenting the overflow risk in the class-level Doxygen (e.g., recommendlong longfor large inputs).
1244-1261: Consider adding a Doxygen comment to theNodestruct.The
Nodestruct packs quite a few fields. A brief inline comment mapping each field to its role (especiallysecond_max/second_minand the lazy tags) would help maintainers.
1263-1312:pull_upunconditionally resets lazy tags on lines 1310–1311.
pull_upis called after recursive updates that alreadypush_down'd the node's lazy (clearing them). The redundant reset is harmless but slightly misleading — it suggestspull_upowns lazy initialisation. Consider adding a brief comment explaining this is intentional defensive coding, or removing the two lines since the invariant is already maintained bypush_down.
1314-1355:push_chmin_to_child/push_chmax_to_childare also called on the node itself (not just children).In
chmin_impl(line 1397) andchmax_impl(line 1416), these functions are invoked withnodeas the argument — the current node, not a child. The namepush_*_to_childis misleading in that context. Consider renaming toapply_chmin/apply_chmax(orapply_chmin_tag/apply_chmax_tag) to clarify that the function applies a clamp operation to any node's statistics and lazy tags.♻️ Renaming suggestion
- void push_chmin_to_child(size_t child, T v) const + void apply_chmin_tag(size_t node, T v) const { - auto & nd = tree(child); + auto & nd = tree(node);- void push_chmax_to_child(size_t child, T v) const + void apply_chmax_tag(size_t node, T v) const { - auto & nd = tree(child); + auto & nd = tree(node);Update all call sites accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tpl_segment_tree.H`:
- Around line 439-445: The swap member is declared unconditionally noexcept but
calls std::swap on identity and op which may throw; change the noexcept
specifier to a conditional expression that reflects whether all swapped members
are nothrow-swappable (e.g. noexcept(std::is_nothrow_swappable_v<decltype(tree)>
&& std::is_nothrow_swappable_v<decltype(n)> &&
std::is_nothrow_swappable_v<decltype(identity)> &&
std::is_nothrow_swappable_v<decltype(op)> ) or use
noexcept(noexcept(std::swap(identity, identity)) && noexcept(std::swap(op, op))
...) so Gen_Segment_Tree::swap, Gen_Lazy_Segment_Tree::swap (and leave
Segment_Tree_Beats::swap as-is if it only swaps noexcept types) only declare
noexcept when the constituent swaps are truly nothrow; update the swap
declarations accordingly.
🧹 Nitpick comments (1)
tpl_segment_tree.H (1)
430-436:values()is O(n log n) but could be O(n) by reading leaves directly.Each
get(i)traverses root-to-leaf. Since the underlying array stores leaf values at known positions, you can extract them in a single O(n) pass. The same applies toGen_Lazy_Segment_Tree::values()(line 1145) andSegment_Tree_Beats::values()(line 1651) — though the lazy variants would need a full push-down first.
Summary by CodeRabbit
New Features
Documentation
Examples
Tests
Chores