perf: cache consensus.size() in loops

[nh13]

Summary

Cache consensus.size() in a local variable instead of calling it repeatedly.

Background

From code review (research.md §2.2, LOW severity):
Multiple calls to consensus.size() in the same scope. While the compiler may optimize this, explicitly caching is clearer.

Changes

• src/caller.cpp: Add const size_t consensus_len = consensus.size() and use it in all loops

Test plan

• CI workflow passes
• Coverage output unchanged

Closes #35

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor
  • Optimized internal performance to reduce redundant computations.
  • Enhanced error detection with warnings for edge case handling to improve robustness.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request optimizes performance in src/caller.cpp by caching the consensus string length in a local variable instead of repeatedly calling consensus.size() in loops. Additionally, bounds checking is introduced to detect and warn when computed coverage indices fall outside vector bounds during pairwise MSA operations, while preserving existing behavior for valid indices.

Changes

Cohort / File(s)	Summary
Consensus length caching and bounds checking src/caller.cpp	Caches consensus.size() as consensus_len and replaces all loop-based calls to consensus.size(); adds warning-level bounds validation for coverage vector access with out-of-bounds index detection during pairwise MSA adjustments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• fix: add bounds check for coverage array access #24: Modifies the same pairwise MSA coverage-access code in src/caller.cpp to compute an index using consensus length and add bounds checking with a warning path.

Poem

🐰 A rabbit hops through loops so fast,

With cached consensus, at long last!

No size() calls to slow the way,

And bounds checks guard us every day! 🎯

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: caching consensus.size() in loops for performance optimization.
Linked Issues check	✅ Passed	The pull request fully implements the requirement from issue #35 to cache consensus.size() in a local variable and use it throughout loops.
Out of Scope Changes check	✅ Passed	All changes are scoped to the objective of caching consensus.size(). The addition of out-of-bounds warning handling is a defensive measure to ensure correctness.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/35-cache-size

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nh13]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/caller.cpp (1)

221-221: Consider using size_t for loop variable consistency.

The loop variable j is uint32_t, but consensus_len is size_t. While biological sequences are unlikely to exceed UINT32_MAX in practice, using size_t would be more consistent and correct.

🔎 Proposed fix for type consistency

-            for (uint32_t j = 0; j < consensus_len; ++j) {
+            for (size_t j = 0; j < consensus_len; ++j) {
                 fprintf(stdout, ",%u", coverage[i * consensus_len + j]);

The same change applies to line 228.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c6187f and 42c87b0.

📒 Files selected for processing (1)

• src/caller.cpp

🔇 Additional comments (3)

src/caller.cpp (3)

203-203: LGTM! Effective performance optimization.

Caching consensus.size() in a local variable eliminates repeated method calls and makes the intent clearer, aligning with the PR objective.

---

206-215: LGTM! Good defensive programming.

The loop correctly uses the cached consensus_len value, and the bounds checking at lines 209-214 adds valuable safety by detecting and warning about out-of-bounds indices while preserving the existing behavior for valid cases.

---

228-229: Same type consistency consideration applies here.

As noted in the previous comment for line 221, using size_t instead of uint32_t for the loop variable j would improve consistency with consensus_len.