-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Optimize Concatenate_iterator to O(1) for random access iterators #9244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Optimize Concatenate_iterator to O(1) for random access iterators #9244
Conversation
Use if constexpr to provide O(1) implementations of operator+, operator-, operator+=, and operator-= when underlying iterators are random access. Falls back to the original O(n) loop for forward/bidirectional iterators. Changes: - Correctly deduce iterator_category using std::common_type to support mixed iterator types. - Add operator-(difference_type) for backwards traversal. - Add operator-= for compound subtraction. - Fix operator+ to handle negative offsets by delegating to operator-. - Fix operator- to handle negative offsets by delegating to operator+. - Add comprehensive tests for O(1) arithmetic operators and mixed iterator categories. This resolves the TODO comments at lines 100 and 116.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR optimizes CGAL::Concatenate_iterator to provide O(1) time complexity for arithmetic operations when the underlying iterators are Random Access Iterators, replacing the previous O(n) loop-based implementations.
Key changes:
- Implements O(1) arithmetic operations (
operator+,operator-,operator+=,operator-=) and distance calculations for random access iterators usingif constexpr - Updates iterator category deduction to use
std::common_type_tto handle mixed iterator types - Adds comprehensive tests for O(1) behavior and mixed iterator categories
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| STL_Extension/include/CGAL/Concatenate_iterator.h | Implements O(1) arithmetic operations with conditional compilation based on iterator category; adds Traits2 typedef and updates iterator_category deduction |
| STL_Extension/test/STL_Extension/test_Concatenate_iterator.cpp | Adds tests for O(1) operations with vectors and regression tests for mixed iterator categories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary of Changes
This PR optimizes$O(1)$ time complexity for arithmetic operations (
CGAL::Concatenate_iteratorto provideoperator+,operator-,operator+=,operator-=) and distance calculations when the underlying iterators are Random Access Iterators (e.g.,std::vector, pointers).Previously, these operations relied on$O(n)$ loops (checking $O(n)$ fallback for Forward or Bidirectional iterators (e.g.,
++or--sequentially). This change utilizes C++17if constexprto conditionally enable the optimized arithmetic logic while preserving the originalstd::list).Key Changes:
operator+andoperator-(offset) to calculate positions immediately by checking range boundaries.operator-(difference) to calculate the distance between two iterators across the concatenated ranges without traversal.iterator_categoryto usestd::common_type_t. This correctly degrades the category to the "lowest common denominator" (e.g., mixing a Random Access iterator with a Bidirectional iterator results in a BidirectionalConcatenate_iterator).operator+=andoperator-=.operator+andoperator-to correctly handle negative arguments by delegating to their inverse counterparts.std::vectorand regression tests for mixed iterator categories.This resolves the
TODOcomments previously located at lines 100 and 116 regarding complexity optimizations.Release Management
STL_ExtensionConcatenate_iterator