Skip to content

prefer ranges algorithms over iterator-pair std calls (library + tests) - #244

Merged
cameroncuster merged 7 commits into
devfrom
use-ranges-algorithms
Jul 12, 2026
Merged

cameroncuster merged 7 commits into
devfrom
use-ranges-algorithms

Conversation

@cameroncuster

@cameroncuster cameroncuster commented Jul 12, 2026

Copy link
Copy Markdown
Member

What

Prefer the ranges:: algorithms over their iterator-pair std::
counterparts for full-range calls, across both the library and the
test suite. The rule: use ranges:: even when all() would work, unless
iterators specifically make more sense.

Conversions applied:

  • iota(all(x), v)ranges::iota(x, v)
  • unique(all(x))begin(ranges::unique(x)) (erase / == end idioms)
  • partition(all(x), pred)begin(ranges::partition(x, pred))
  • remove_if(all(x), pred)begin(ranges::remove_if(x, pred)) (erase)
  • next_permutation(all(x))ranges::next_permutation(x).found

ranges::unique / ranges::partition / ranges::remove_if return a
subrange, so begin(...) yields the iterator the erase / offset logic
needs; ranges::next_permutation returns a struct whose .found drives
the do/while.

Why

The consistency signal is the goal, not raw source length. #221
("standardize on c++20: ranges/views where shorter") used a
shorter-in-this-repo heuristic and, on that basis, reverted the
subrange-returning algos (unique / remove_if / partition) back to
iterator form. This PR intentionally revisits that for one reason:

  • The all(x) macro expands on main. When KACTL macros are
    expanded for the published notebook, all(x) becomes
    begin(x), end(x), so the iterator form unique(begin(x), end(x)) is
    actually longer than begin(ranges::unique(x)). The
    "shorter" heuristic that motivated the standardize on c++20: ranges/views and <bit> used consistently #221 revert inverts once the
    macro is gone — so the ranges:: form is the one that stays clean in
    the shipped code.
  • Consistency with existing style. The library and tests already use
    ranges:: algorithms extensively (e.g. ranges::sort everywhere);
    these conversions close the remaining full-range gaps rather than
    leaving a mix of idioms.

Scope / what is intentionally NOT changed

  • std::partial_sum and std::accumulate — no ranges::
    numeric-reduction equivalents in C++23 (ranges::fold_left would be
    longer and change the idiom: accumulate(all(v), 0)
    fold_left(v, 0, plus{})), so these stay as-is.
  • Partial-range calls (iterator sub-ranges such as
    lower_bound(begin(sa) + lo, ...), find_if(rank + begin(mat), ...),
    partition(el, er, ...), remove_if(idx + begin(upd_st), ...)) — read
    more clearly with explicit iterators.
  • Member functions named find / count — not std:: algorithms.
  • Scalar std::min / std::max.

Files

library/ (12 sites, 12 files): line_tree, mst,
offline_incremental_scc, complement_graph_ccs, calc_sieve,
calc_linear_sieve, lcs_dp, longest_palindrome_query, suffix_array (2),
suffix_array_short (iota); virtual_tree (unique); edge_cd (partition).

tests/ (10 files): bit_ordered_set, kth_smallest_pst, mode_query,
sa_sort_pairs, seg_tree_find (unique); mono_st_asserts,
offline_incremental_scc, sa_sort_pairs (iota); permutation_tree_small,
rmq_small_n (iota + next_permutation); single_matching_bs (remove_if).

Testing

Syntax-checked all affected library-checker/aizu tests locally with
clang++ -std=c++2b -fsyntax-only — all pass.

Behaviorally ran (exit 0, all asserts pass):

  • edge_cd_small_trees — exhaustive over all trees on 2–7 nodes
    (ranges::partition).
  • lca_all_methods_aizu — includes compress_tree_asserts, comparing
    compress_tree against KACTL's reference over random subsets plus the
    empty-subset edge case (ranges::unique).
  • permutation_tree_small and rmq_small_n — exhaustive over all
    permutations of size 1–8 (ranges::next_permutation(...).found,
    ranges::iota).

Convert the full-range std::iota(begin, end, v) calls to
ranges::iota(range, v). This favors the ranges algorithms already used
throughout the library and removes an all(...) macro expansion on main.

Partial-range calls (e.g. iterator sub-ranges) and std::partial_sum are
left as-is: partial_sum has no ranges equivalent, and partial ranges read
more clearly with explicit iterators.
Convert the remaining full-range std algorithm calls to their ranges::
equivalents:
- virtual_tree: unique(all(subset)) -> begin(ranges::unique(subset))
- edge_cd: partition(all(g[u]), ...) -> begin(ranges::partition(g[u], ...))

ranges::unique / ranges::partition return a subrange, so begin(...) yields
the iterator the erase / offset logic needs.

Partial-range calls (iterator sub-ranges) and std::partial_sum are still
left as-is: partial_sum has no ranges equivalent, and partial ranges read
more clearly with explicit iterators.
Apply the same "prefer ranges" convention to the test suite:
- iota(all(x), v)            -> ranges::iota(x, v)
- unique(all(x))            -> begin(ranges::unique(x)) (erase / == end idiom)
- remove_if(all(x), pred)   -> begin(ranges::remove_if(x, pred)) (erase idiom)
- next_permutation(all(x))  -> ranges::next_permutation(x).found

accumulate(all(x), ...) is left as-is: there is no ranges::accumulate in
C++23 (ranges::fold_left would change the idiom), matching how partial_sum
is left in the library.
@cameroncuster
cameroncuster merged commit a76ca2f into dev Jul 12, 2026
@cameroncuster
cameroncuster deleted the use-ranges-algorithms branch July 12, 2026 19:50
cameroncuster added a commit that referenced this pull request Jul 19, 2026
cppcheck flags mixing iterators from two different expressions when the
erase-remove idiom is written as erase(begin(ranges::unique(x)), end(x)):
the first iterator comes from the returned subrange while the second
comes from the container. Store the returned subrange in a local and
erase [begin, end) of that single subrange, which is semantically
identical (the subrange spans the trailing removed elements) and keeps
both iterators from one expression.

Applies to ranges::unique and ranges::remove_if call sites introduced in
the ranges-algorithm migration (#244):

- library/trees/extra_members/virtual_tree.hpp
- tests .../bit_ordered_set.test.cpp
- tests .../kth_smallest_pst.test.cpp
- tests .../mode_query.test.cpp
- tests .../sa_sort_pairs.test.cpp
- tests .../single_matching_bs.test.cpp
cameroncuster added a commit that referenced this pull request Jul 19, 2026
Keep the terse erase-remove one-liners from the ranges migration (#244)
and silence cppcheck's mismatchingContainerExpression false positive
instead. The warning fires because a range algorithm returns a subrange,
so the first iterator's source expression (ranges::unique(x)) differs
syntactically from the second (end(x)) even though end(subrange) is
end(x) by construction.

Reverts the earlier local-variable rewrite and adds line-anchored
suppressions for the six affected call sites.
cameroncuster added a commit that referenced this pull request Jul 19, 2026
* fix pre-existing dev clang-format violations

Reformat 5 files that violated the dev clang-format config
(ColumnLimit 59) so the grep_clangformat_cppcheck CI job passes.
Line-wrapping only; no behavior change.

- library/dsu/kruskal_tree.hpp
- library/strings/longest_common_subsequence/lcs_dp.hpp
- library/strings/manacher/manacher.hpp
- tests/.../data_structures/bit_ordered_set.test.cpp
- tests/.../strings/sa_sort_pairs.test.cpp

* [auto-verifier] verify commit 35b1fb2

* sync stale cppcheck suppression line numbers

The line-anchored knownConditionTrueFalse suppressions had drifted
out of sync with their source lines, causing both unmatchedSuppression
and now-unsuppressed errors in the cppcheck CI step:

- kruskal_tree.hpp: 13 -> 15 (shifted by the clang-format reflow in
  the previous commit)
- suffix_array.hpp: 62 -> 63 (pre-existing drift)
- suffix_array_short.hpp: 34 -> 35 (pre-existing drift)

* fix cppcheck mismatchingContainerExpression on ranges erase-remove

cppcheck flags mixing iterators from two different expressions when the
erase-remove idiom is written as erase(begin(ranges::unique(x)), end(x)):
the first iterator comes from the returned subrange while the second
comes from the container. Store the returned subrange in a local and
erase [begin, end) of that single subrange, which is semantically
identical (the subrange spans the trailing removed elements) and keeps
both iterators from one expression.

Applies to ranges::unique and ranges::remove_if call sites introduced in
the ranges-algorithm migration (#244):

- library/trees/extra_members/virtual_tree.hpp
- tests .../bit_ordered_set.test.cpp
- tests .../kth_smallest_pst.test.cpp
- tests .../mode_query.test.cpp
- tests .../sa_sort_pairs.test.cpp
- tests .../single_matching_bs.test.cpp

* [auto-verifier] verify commit 0152415

* suppress cppcheck mismatchingContainerExpression instead of rewriting

Keep the terse erase-remove one-liners from the ranges migration (#244)
and silence cppcheck's mismatchingContainerExpression false positive
instead. The warning fires because a range algorithm returns a subrange,
so the first iterator's source expression (ranges::unique(x)) differs
syntactically from the second (end(x)) even though end(subrange) is
end(x) by construction.

Reverts the earlier local-variable rewrite and adds line-anchored
suppressions for the six affected call sites.

---------

Co-authored-by: GitHub <noreply@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants