Rewriting segmented_search_n for more compact and efficient implementation.

include/boost/container/experimental/segmented_search_n.hpp

@@ -32,14 +32,14 @@ FwdIt segmented_search_n
 
 namespace detail_algo {
 
-// search_n_scan_segment scans a single (sub-)segment for a run of `count`
-// consecutive elements equal to `value`, threading cross-segment state
-// through the `consecutive` parameter. The result is a segtrio:
+// search_n_scan_segment scans a single (sub-)segment for a run of "count"
+// consecutive elements equal to "value", threading cross-segment state
+// through the "consecutive" parameter. The result is a segtrio:
 //
 //   first  : true if the full run was completed inside this scan
 //   second : current consecutive count at end of scan (carries to next segment)
 //   third  : start of the run when first==true; otherwise the position where
-//            a new run started inside this scan, or `lend` (sentinel) when
+//            a new run started inside this scan, or "lend" (sentinel) when
 //            no new run started in this scan (i.e. the in-progress run, if
 //            any, started in a previous segment).
 //
@@ -49,212 +49,175 @@ namespace detail_algo {
 // (non-segmented, random-access, same-typed iterators)
 // Boyer-Moore-style "skip-by-count" scan with cross-segment carry-over.
 //
-// Phase 1: if `consecutive` carries a partial run from the previous
-//   segment, probe at `lcur + ncheck - 1` (the last position needed to
+// Phase 1: if "consecutive" carries a partial run from the previous
+//   segment, probe at "lcur + ncheck - 1" (the last position needed to
 //   complete the carry-over, clamped by the segment) and verify
 //   backward on a hit. On success return; on a mismatch reset
-//   `consecutive` to 0 and fall through to phase 2 from the position
-//   past the mismatch -- with `consecutive > 0`, no in-segment run can
+//   "consecutive" to 0 and fall through to phase 2 from the position
+//   past the mismatch -- with "consecutive > 0", no in-segment run can
 //   start at or before the probe, so the entire range up to the probe
 //   can be skipped without inspection on a probe miss.
-// Phase 2: probe at `lcur + (count - 1)`. On match, verify backward to
-//   find the run start; on mismatch advance the probe by `count`
+// Phase 2: probe at "lcur + (count - 1)". On match, verify backward to
+//   find the run start; on mismatch advance the probe by "count"
 //   positions instead of 1 (Boyer-Moore). The mismatched element cannot
 //   belong to any complete run that fits in [probe - count + 1, probe],
 //   so the next candidate run can only start past it. This makes the
 //   scan O(n / count) on the value-sparse portion of the range.
-// Phase 3: once `probe >= lend`, scan the tail to detect a partial
+// Phase 3: once "probe >= lend", scan the tail to detect a partial
 //   trailing run that the next segment may extend. Bounded by phase 2's
-//   exit position: `*(probe - count) != value` is known, so the scan
-//   range is (probe - count, lend) of length `lend - probe + count - 1`,
+//   exit position: "*(probe - count) != value" is known, so the scan
+//   range is (probe - count, lend) of length "lend - probe + count - 1",
 //   which is in [0, count - 1] -- often well below count - 1, and
-//   exactly 0 when the last probe landed at `lend - 1` and missed.
+//   exactly 0 when the last probe landed at "lend - 1" and missed.
 template <class LocalIter, class Size, class T>
 segtrio<bool, Size, LocalIter> search_n_scan_segment
-   (LocalIter lcur, LocalIter lend,
+   (LocalIter lcur, LocalIter const lend,
     Size consecutive, Size count, const T& BOOST_RESTRICT value,
     const non_segmented_iterator_tag &, const std::random_access_iterator_tag &)
 {
    typedef segtrio<bool, Size, LocalIter> result_t;
    typedef typename iterator_traits<LocalIter>::difference_type difference_type;
 
-   LocalIter match_start = lend;
-
-   // Phase 1: extend a partial run carried from the previous segment.
+   // Phase 1: extend a partial run (consecutive > 0) carried from the previous segment.
    //
-   // Probe at `lcur + ncheck - 1` (the last position the carry-over needs to
-   // cover) and, on match, verify backward to `lcur`. This lets us discard
-   // the entire `[lcur, probe - 1]` range on a single mismatch at `probe`:
+   // Single-cursor verify mirroring phase 2's V3 shape: `lcur` is stepped
+   // to "verify_probe = original_lcur + ncheck - 1" (the last position the
+   // carry-over needs to cover) and the inner loop sweeps the ncheck
+   // positions [original_lcur, verify_probe] backward via "--lcur".
    //
-   // - When ncheck == need (segment can host the completion), every run that
-   //   could complete the carry-over covers position `probe`, and so do all
-   //   in-segment runs starting at `s in [lcur, probe]` (because the smallest
-   //   in-segment start whose run does NOT cover `probe` would be at
-   //   `s = probe - count + 1 = lcur - consecutive`, which is impossible
-   //   when consecutive > 0). So `*probe != value` rules them all out and we
-   //   may jump `lcur` to `probe + 1` without inspecting `[lcur_old, probe - 1]`.
-   //
-   // - When ncheck == avail < need (segment too short to complete), `probe`
-   //   is `lend - 1`, and `*probe != value` makes any phase-3 trailing run
-   //   length 0 (the very last element is non-matching), which is what
-   //   forward Phase 1 would have observed anyway.
+   // OOB safety: exactly ncheck - 1 decrements (not ncheck) are executed
+   // on the full-match path, exiting via "if (--left == 0)" before the
+   // final --lcur. This avoids forming "original_lcur - 1" -- UB for raw
+   // pointers, which the segmented deque path always supplies as
+   // chunk-start pointers.
    if(consecutive > 0) {
+      // Handles the case where the segment too short to complete the run.
       const difference_type need   = static_cast<difference_type>(count - consecutive);
       const difference_type avail  = lend - lcur;
       const difference_type ncheck = need < avail ? need : avail;
-      if(ncheck == 0)
-         // Empty segment; carry-over preserved for the next one.
-         return result_t(false, consecutive, match_start);
-      const LocalIter probe = lcur + (ncheck - 1);
-      if(*probe == value) {
-         // Backward verify ncheck - 1 positions before probe.
-         LocalIter back = probe;
-         Size left = static_cast<Size>(ncheck - 1);
-         BOOST_CONTAINER_UNROLL(4)
-         while(left > 0) {
-            --back;
-            if(!(*back == value)) {
-               // Mismatch at `back`. Carry-over broken; reset and let
-               // phase 2 resume from the position past `back`. Using
-               // `++back` (then `lcur = back`) instead of `back + 1`
-               // lets iterator types with a fast in-step increment
-               // avoid the generic `it + n` arithmetic path.
-               consecutive = 0;
-               ++back;
-               lcur = back;
-               goto mismatch;
-            }
-            --left;
+      if(BOOST_UNLIKELY(ncheck == 0))   // Empty segment; carry-over preserved for the next one.
+         return result_t(false, consecutive, lend);
+
+      // Step "lcur" itself to the verify probe position; no separate cursor.
+      lcur += (ncheck - 1);
+      Size left = static_cast<Size>(ncheck);
+      BOOST_CONTAINER_UNROLL(4)
+      do {
+         if(!(*lcur == value))
+            goto phase1_mismatch; // Mismatch, jump to cleanup logic
+         if (--left == 0) {
+            goto full_match;
          }
-         // Full backward verify: every position in [lcur, probe] matches.
-         consecutive = static_cast<Size>(consecutive + ncheck);
-         return result_t(consecutive == count, consecutive, match_start);
-      }
-      else {
-         // *probe != value. Skip past the entire check range without
-         // inspecting [lcur, probe - 1] (justification above).
-         // `lcur = probe; ++lcur` instead of `probe + 1` for the same
-         // increment-vs-arithmetic reason as above.
-         consecutive = 0;
-         lcur = probe;
-         ++lcur;
-      }
-      mismatch:   ;
+         --lcur;
+      } while(true);
+
+      full_match:
+      // Full backwards verification: run start = original_lcur.
+      // Put "start of the run" return to sentinel so the outer
+      // match_start is not updated.
+      consecutive += static_cast<Size>(ncheck);
+      return result_t(consecutive == count, consecutive, lend);
+
+      // Mismatch, reset consecutive counter and step past the failing position.
+      phase1_mismatch:
+      consecutive = 0;
+      ++lcur;
    }
 
    const difference_type dcount    = static_cast<difference_type>(count);
    const difference_type remaining = lend - lcur;
 
-   // Phase 3 bound. Default: scan the whole segment from `lcur`. This is
-   // only used when phase 2 is skipped because the segment is shorter than
-   // `count`; otherwise phase 2 tightens it further on exit.
-   difference_type tail_max;
-
-   // Phase 2: skip-by-count probing -- only meaningful if a full run fits.
+   // Phase 2: Only meaningful if a full run fits (dcount <= remaining)
+   //          No partial run. Skip-by-count probing.
    //
-   // Driven by a running probe iterator, advanced in place by `dcount` on
-   // a miss and by `left` after a backward-verify mismatch. For chunked
-   // local iterators (e.g. raw pointer into a deque chunk) the in-chunk
-   // advance collapses to a single register add, with chunk-boundary work
-   // amortized over chunk_size/count probes; an index-based form
-   // `probe = lcur + pidx` would force the chunk-base load on the critical
-   // path of every probe.
+   // Single-cursor Boyer-Moore: at the top of each outer iteration `lcur`
+   // sits at the verify probe position. The inner verify checks *lcur
+   // (its first iteration replaces an outer "if(*probe == value)" gate)
+   // then walks down via "--lcur", sweeping the count positions
+   // [verify_probe - (count - 1), verify_probe].
+   //
+   // After an inner mismatch at iter K (K in [1, count]) `lcur` lands at
+   // LKNM = verify_probe - (K - 1); the next outer iter's verify_probe is
+   // verify_probe + (count - K + 1), so the advance from LKNM is
+   // (count - K + 1) + (K - 1) = count -- a constant `dcount`, regardless
+   // of K. The BM acceleration is entirely encoded in how far the inner
+   // loop decrements `lcur` before mismatching, and the outer step is just
+   // dcount.
+   //
+   // OOB safety: `lend_safe = lend - dcount` is precomputed once (well-formed
+   // because dcount <= remaining = lend - original_lcur). The guard
+   // `lcur >= lend_safe` is a pointer comparison only -- no new pointer is
+   // ever formed past `lend`. When `lcur < lend_safe`, `lcur + dcount`
+   // lands at most at `lend - 1`, which is dereferenceable.
    if(dcount <= remaining) {
-      // dcount <= remaining => probe = lcur + (dcount - 1) is in [lcur, lend),
-      // so the loop body always executes at least once: use do/while.
-      LocalIter probe = lcur;
-      probe += (dcount - 1);
-      do {
-         if(*probe == value) {
-            // Backward verify count - 1 elements before probe.
-            LocalIter back = probe;
-            Size left = static_cast<Size>(count - 1);
-            BOOST_CONTAINER_UNROLL(4)
-            do {  //count is always >= 2 because it's tested in the outer function
-               --back;
-               if(!(*back == value))
-                  goto back_mismatch;
-               --left;
-            } while (left > 0);
-            match_start = back;
-            return result_t(true, count, match_start);
+      // First verify probe: original_lcur + dcount - 1, in [original_lcur, lend).
+      lcur += (dcount - 1);
+      const LocalIter lend_safe = lend - dcount;  // largest lcur that can advance
+      while (true) {
+         // Inner verify sweeps the count positions [verify_probe - (count - 1),
+         // verify_probe] backward. Doing exactly count - 1 decrements (not
+         // count) avoids forming "original_lcur - 1" when the run lands
+         // right at the start of the segment.
+         Size left = count;
+         BOOST_CONTAINER_UNROLL(4)
+         do {  //count is always >= 2 because it's tested in the outer function
+            if(!(*lcur == value))
+               goto phase2_mismatch;
+            if (--left == 0)
+               // Run start: lcur = verify_probe - (count - 1).
+               return result_t(true, count, lcur);
+            --lcur;
+         } while (true);
 
-            // Mismatch at `back`; smallest possible new candidate start is
-            // back + 1, so next probe is back + count. With back at
-            // probe - (count - left), advancing by `left` lands the running
-            // probe at the next candidate's verification position.
-            back_mismatch:
-            probe += static_cast<difference_type>(left);
-            continue;
-         }
-         else {
-            probe += dcount;
-         }
-      } while(probe < lend);
-
-      // Phase-2 exit invariant: the last formed probe was at `probe - dcount`
-      // (no-match branch) or at `probe - count` (back-mismatch branch); both
-      // carry a confirmed `*!= value`. Therefore no partial trailing run can
-      // start at or before that position, and phase 3 only needs to scan a
-      // tail of length `(lend - probe) + (dcount - 1)`, which is always in
-      // [0, count - 1].
-      tail_max = (lend - probe) + (dcount - 1);
-   }
-   else {
-      // dcount > remaining => (dcount - 1) >= remaining, so the bound
-      // collapses to `remaining` (scan the whole segment from `lcur`).
-      tail_max = remaining;
+         phase2_mismatch: ;
+         // lcur at LKNM (failed check). OOB-safe advance to next verify probe.
+         if (lcur >= lend_safe)
+            break;
+         lcur += dcount;
+      }
+      // Exit: lcur at LKNM. The phase-3 tail scan starts one past it.
+      ++lcur;
    }
 
    // Phase 3: partial trailing run that could extend into the next segment.
    //
-   // Mirrors phase 2's verify-loop pattern: a do/while drives `--tail` + probe
-   // and a `goto` lifts the mismatch fix-up out of the hot body, leaving a
-   // single straight-line path through the inner loop. This:
-   //   * eliminates the top-of-loop `tail != tail_lo` check on entry,
-   //   * removes the `++tail` undo from the hot body (it runs once at the
-   //     mismatch label), and
-   //   * lets the full-run fall-through report `tail_max` directly without
-   //     a final `lend - tail` subtraction.
+   // Scans [lcur, lend) backward. Mirrors phase 2's verify-loop pattern
    {
-      const LocalIter tail_lo = lend - tail_max;
-      LocalIter       tail    = lend;
-      if (tail == tail_lo)         // tail_max == 0: empty tail, nothing to scan.
-         return result_t(false, 0, match_start);
+      LocalIter tail = lend;
+      if (tail == lcur)            // empty tail, nothing to scan.
+         return result_t(false, 0, lend);
 
       BOOST_CONTAINER_UNROLL(4)
       do {
          --tail;
          if (!(*tail == value))
             goto tail_mismatch;
-      } while (tail != tail_lo);
+      } while (tail != lcur);
 
-      // Full tail run: every element in [tail_lo, lend) equals value.
-      // tail == tail_lo and tail_max > 0, so the run is non-empty.
-      match_start = tail;
-      return result_t(false, static_cast<Size>(tail_max), match_start);
+      // Full tail run: every element in [lcur, lend) equals value.
+      // tail == lcur and (lend - lcur) > 0, so the run is non-empty.
+      return result_t(false, static_cast<Size>(lend - tail), tail);
 
-      // Mismatch at `tail`; the partial run starts one past it. On a
-      // first-iteration mismatch (`*(lend - 1) != value`) the post-`++tail`
-      // value collapses to `lend`, which the caller already interprets as
-      // "no match position" via `r.third != lend`, so the assignment is
-      // safe to do unconditionally and the `tail_run > 0` guard is
-      // unnecessary.
+      // Mismatch at "tail"; the partial run starts one past it. On a
+      // first-iteration mismatch ("*(lend - 1) != value") the post-"++tail"
+      // value collapses to "lend", which the caller already interprets as
+      // "no match position" via "r.third != lend", so the assignment is
+      // safe to do unconditionally.
       tail_mismatch:
-      ++tail;
-      match_start = tail;
+      LocalIter match_start = tail;
+      ++match_start;
       return result_t(false, static_cast<Size>(lend - tail), match_start);
    }
 }
 
-// Initial value for `match_start` used by the forward scan below.
+// Initial value for "match_start" used by the forward scan below.
 //
 // When invoked with same-typed iterators (the recursive segmented caller's
-// path) we return `lend` so the caller can detect "no new run started in
-// this scan" via `r.third != lend`. With a real sentinel this trick isn't
+// path) we return "lend" so the caller can detect "no new run started in
+// this scan" via "r.third != lend". With a real sentinel this trick isn't
 // available (Sent cannot be assigned to LocalIter), but it isn't needed
 // either: that path is reached only at the top level, where the caller only
-// consults `r.third` when `r.first == true`, in which case `match_start`
+// consults "r.third" when "r.first == true", in which case "match_start"
 // has been set to a real position inside the loop.
 template <class LocalIter>
 BOOST_CONTAINER_FORCEINLINE
@@ -267,7 +230,7 @@ search_n_init_match_start(LocalIter lcur, Sent) { return lcur; }
 
 // Forward scan: handles both the same-typed (recursive segmented caller) and
 // the real-sentinel (top-level) cases with one body. The only behavioral
-// difference -- the initial value of `match_start` -- is folded into the
+// difference -- the initial value of "match_start" -- is folded into the
 // search_n_init_match_start helper above.
 template <class LocalIter, class Sent, class Size, class T, class Tag, class Cat>
 BOOST_CONTAINER_FORCEINLINE
@@ -394,7 +357,7 @@ FwdIt search_n_top_dispatch
 }
 
 // Top-level non-segmented RA fast path. Phase 1 (carry-over from a previous
-// segment) is dead because `consecutive == 0` at the top level. Phase 3
+// segment) is dead because "consecutive == 0" at the top level. Phase 3
 // (trailing partial run that the next segment may extend) is dead because
 // there is no next segment. Reduces to pure Boyer-Moore skip-by-count
 // probing, returning the iterator directly without going through the
@@ -405,39 +368,68 @@ FwdIt search_n_top_dispatch
 // iterators (e.g. wrapped deque_iterator) this collapses to a single
 // register add inside a chunk, with chunk-boundary work amortized over
 // chunk_size/count probes -- vastly cheaper than recomputing
-// `first + pidx` (which forces a chunk-base load on the critical path of
+// "first + pidx" (which forces a chunk-base load on the critical path of
 // every probe).
 template <class FwdIt, class Size, class T>
 BOOST_CONTAINER_FORCEINLINE
 FwdIt search_n_top_dispatch
-   (FwdIt first, FwdIt last, Size count, const T& BOOST_RESTRICT value,
+   (FwdIt first, const FwdIt last, Size count, const T& BOOST_RESTRICT value,
     const non_segmented_iterator_tag &, const std::random_access_iterator_tag &)
 {
    typedef typename iterator_traits<FwdIt>::difference_type difference_type;
    const difference_type dcount = static_cast<difference_type>(count);
-   // dcount <= remaining guaranteed by the search_n_range_shorter_than guard.
-   FwdIt probe = first + (dcount - 1);
-   do {
-      if (*probe == value) {
-         FwdIt back = probe;
-         Size left = static_cast<Size>(count - 1);
-         BOOST_CONTAINER_UNROLL(4)
-         do {
-            --back;
-            if (!(*back == value))
-               goto back_mismatch;
-            --left;
-         } while (left > 0);
-         return back;
-         back_mismatch:
-         probe += static_cast<difference_type>(left);
-         continue;
-      }
-      else {
-         probe += dcount;
-      }
-   } while (probe < last);
-   return last;
+   // dcount <= last - first guaranteed by search_n_range_shorter_than.
+   //
+   // Single-cursor Boyer-Moore: `first` (a by-value parameter, private to
+   // this frame) is repurposed as the running probe. After the initial
+   // advance to "first + dcount - 1" it no longer means "start of the
+   // search range" -- it tracks the verify probe through the rest of the
+   // function. `last_safe = last - dcount` is precomputed before the
+   // mutation while `first` still names the original lower bound.
+   //
+   // The inner verify checks *first (its first iteration replaces the old
+   // "if (*probe == value)" outer check) then walks down via "--first",
+   // sweeping the count positions [verify_probe - (count - 1), verify_probe].
+   //
+   // After an inner mismatch at iter K (K in [1, count]) `first` lands at
+   // LKNM = verify_probe - (K - 1); the next outer iter's verify_probe is
+   // verify_probe + (count - K + 1), so the advance from LKNM is
+   // (count - K + 1) + (K - 1) = count -- a constant `dcount`, regardless
+   // of K. So the BM acceleration is entirely encoded in how far the inner
+   // loop decrements `first` before mismatching, and the outer step is just
+   // dcount.
+   //
+   // OOB safety: the guard `first >= last_safe` is a pure pointer
+   // comparison -- no new pointer is ever formed past `last`. When
+   // `first < last_safe`, `first + dcount` lands at most at `last - 1`,
+   // which is dereferenceable.
+   const FwdIt last_safe = last - dcount;  // largest probe pos that can advance
+   first += (dcount - 1);                  // first becomes the verify probe
+   while (true) {
+      // Inner verify sweeps the count positions [verify_probe - (count - 1),
+      // verify_probe] backward. Doing exactly count - 1 decrements (not
+      // count) avoids forming "original_first - 1" when the run lands right
+      // at the start of the range -- the OLD `back = probe; Size left =
+      // count - 1` shape, but with `first` itself as the cursor.
+      Size left = count;
+      BOOST_CONTAINER_UNROLL(4)
+      do {
+         if (!(*first == value))
+            goto probe_mismatch;
+         if (--left == 0)
+            return first;             // run start: first = verify_probe - (count - 1)
+         --first;
+      } while (true);
+
+      probe_mismatch: ;
+      // first at LKNM (failed check). OOB-safe advance to next verify probe:
+      // last_safe = last - dcount is precomputed, so the test forms no new
+      // pointer. When first < last_safe, first + dcount lands at most at
+      // last - 1, which is dereferenceable.
+      if (first >= last_safe)
+         return last;
+      first += dcount;
+   }
 }
 
 } // namespace detail_algo
@@ -468,7 +460,7 @@ inline FwdIt segmented_search_n
       return last;   
    else {
       // count >= 2. Tag-dispatched: non-segmented RA top-level calls take a Phase-2-only
-      // fast path (Phase 1 is dead because `consecutive == 0` and Phase 3
+      // fast path (Phase 1 is dead because "consecutive == 0" and Phase 3
       // is dead because there is no next segment). Otherwise (segmented,
       // forward-only, sentinel) the generic overload delegates to
       // search_n_scan_segment + segtrio unwrap.