Fix DDS_AB_STATS build by binding thrp in apply_ab_tt_lookup

[tameware]

Summary

• Fix --define=ab_stats=true compile failure: apply_ab_tt_lookup used AB_COUNT without a thrp.
• Add ABstats::GetPosCount plus unit/integration coverage for the TT-lookup counter path.
• Add a Linux CI step that builds //library/src:dds with --define=ab_stats=true.

Test plan

• bazelisk build --define=ab_stats=true //library/src:dds
• bazelisk test --define=ab_stats=true //library/tests/ab_search:tt_lookup_test
• bazelisk test //library/tests/ab_search:ab_stats_test
• Confirm Linux CI runs the new ab_stats define build step

Made with Cursor

[zzcgumn]

Good catch.

[Copilot]

Pull request overview

This PR fixes a --define=ab_stats=true compile failure by ensuring apply_ab_tt_lookup() binds a ThreadData* thrp for the AB_COUNT macro, adds a small ABstats query helper (GetPosCount), and extends CI/tests to exercise the TT-lookup counter path under DDS_AB_STATS.

Changes:

• Bind ThreadData* thrp in apply_ab_tt_lookup() so AB_COUNT(...) compiles under DDS_AB_STATS.
• Add ABstats::GetPosCount() plus new unit/integration tests validating the TT-lookup counter behavior.
• Add a Linux CI build step for //library/src:dds with --define=ab_stats=true and document the gated diagnostic build steps.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
library/src/ab_search.cpp	Binds thrp in apply_ab_tt_lookup() and uses it in the AB-hits dump path.
library/src/ab_stats.hpp	Declares ABstats::GetPosCount(int) for inspecting per-place counts.
library/src/ab_stats.cpp	Implements ABstats::GetPosCount(int).
library/tests/ab_search/tt_lookup_test.cpp	Adds DDS_AB_STATS-gated tests asserting TT lookup hit/miss counter behavior.
library/tests/ab_search/BUILD.bazel	Adds the new ab_stats_test target.
library/tests/ab_search/ab_stats_test.cpp	Adds unit tests for ABstats counter increment/query behavior.
docs/BUILD_SYSTEM.md	Updates CI documentation to mention the new ab_stats define build.
.github/workflows/ci_linux.yml	Adds a CI build step compiling //library/src:dds with --define=ab_stats=true.

Suppressed comments (3)

library/tests/ab_search/ab_stats_test.cpp:21

• ABstats::Reset() does not initialize the cumulative counters (sumCum, sumCumWeighted), but IncrPos() increments them. Call ResetCum() here as well to avoid incrementing indeterminate values.

  ABstats stats;
  stats.Reset();

  stats.IncrPos(AB_MAIN_LOOKUP, /*side=*/true, /*depth=*/20);

library/tests/ab_search/ab_stats_test.cpp:31

• ABstats::Reset() leaves the cumulative counters untouched; for a freshly constructed ABstats those fields are indeterminate, and later IncrPos() would increment them. Even though this test only reads sum, calling ResetCum() keeps the object in a fully initialized state.

  ABstats stats;
  stats.Reset();

library/tests/ab_search/tt_lookup_test.cpp:159

• Same as above: apply_ab_tt_lookup() increments cumulative counters via IncrPos(), so resetting only the non-cumulative fields is incomplete for a self-contained test. Reset both ABStats and its cumulative counters.

  ThreadData* thrp = ctx_->thread_ptr();
  ASSERT_NE(thrp, nullptr);
  thrp->ABStats.Reset();

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (3)

library/src/ab_stats.cpp:27

• ABstats::ResetCum() does not initialize ABnodesCum.list[], but IncrNode() increments that array and reporting reads it. Adding ResetCum() to the constructor still leaves ABnodesCum.list[] indeterminate, which can lead to UB and incorrect stats. Initialize the ABnodesCum.list[] entries to 0 during construction (or as part of ResetCum()).

ABstats::ABstats()
{
  ABstats::Reset();
  ABstats::ResetCum();
  ABstats::SetNames();

.github/workflows/ci_linux.yml:59

• CI currently compiles the DDS_AB_STATS-gated code path, but it does not run the tests that are only enabled when --define=ab_stats=true (e.g. the new TT lookup hit counter assertions). Adding a targeted bazelisk test --define=ab_stats=true ... step will prevent the stats-only tests from bitrotting.

      # 7️⃣ Build with DDS_AB_STATS (off by default; compile-gated AB counters)
      - name: Build with ab_stats define
        run: bazelisk build --define=ab_stats=true --verbose_failures //library/src:dds

docs/BUILD_SYSTEM.md:142

• The updated CI table row is a bit ambiguous because it reads like a single command with two flags. Since the workflow runs two separate build commands, it would be clearer to spell out both full commands in the table cell.

| `ci_linux.yml` | `build_and_test` | also `bazelisk build --define=ab_stats=true //library/src:dds` and `--define=scheduler=true` (gated diagnostic paths) |

[tameware]

Addressed the latest Copilot notes:

1. ResetCum() now zeroes ABnodesCum.list[] (covered by IncrNodeAfterConstructionUsesZeroedCumNodeList).
2. Linux CI runs bazelisk test --define=ab_stats=true for the ab_stats TT-lookup tests.
3. Docs table lists the separate define build/test commands explicitly.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

[tameware]

One more review please, @zzcgumn. Copilot found some items that seemed to deserve fixing. Cursor was happy to deal with them.