Skip to content

Fix by-reference capture of local variable in solve.cu - #1214

Merged
rapids-bot[bot] merged 1 commit into
mainfrom
mlubin-patch-1
May 14, 2026
Merged

Fix by-reference capture of local variable in solve.cu#1214
rapids-bot[bot] merged 1 commit into
mainfrom
mlubin-patch-1

Conversation

@mlubin

Copy link
Copy Markdown
Contributor

early_fj_start is a local variable and should be captured by value since the callback outlives the local scope.

early_fj_start is a local variable and should be captured by value since the callback outlives the local scope.
@mlubin
mlubin requested a review from hlinsenMay 14, 2026 01:07
@mlubin
mlubin requested a review from a team as a code ownerMay 14, 2026 01:07
@mlubinmlubin added the bug Something isn't working label May 14, 2026
@mlubin
mlubin requested a review from aliceb-nvMay 14, 2026 01:07
@mlubinmlubin added the non-breaking Introduces a non-breaking change label May 14, 2026
@coderabbitai

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1a1f6739-5795-4c81-8a3b-803e707ce5bc

📥 Commits

Reviewing files that changed from the base of the PR and between 20e15cd and 9518f99.

📒 Files selected for processing (1)
  • cpp/src/mip_heuristics/solve.cu

📝 Walkthrough

Walkthrough

The MIP solving helper updates a lambda capture in early_fj_callback to capture early_fj_start by value instead of by reference, preventing potential lifetime issues when the callback executes asynchronously or after the referenced variable scope ends.

Changes

Lambda Capture Mode Update

Layer / File(s)Summary
Early feedback callback variable capture
cpp/src/mip_heuristics/solve.cu
The early_fj_callback lambda's capture of early_fj_start is changed from reference (&early_fj_start) to value (early_fj_start) to ensure safe lifetime handling.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly and specifically describes the main change: fixing a by-reference capture issue with a local variable in solve.cu, which matches the actual code modification.
Description check✅ PassedThe description is directly related to the changeset, explaining the rationale for the fix by noting that early_fj_start is a local variable that should be captured by value since the callback outlives the local scope.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mlubin-patch-1

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

@mlubin

Copy link
Copy Markdown
ContributorAuthor

/merge

@rapids-bot
rapids-botBot merged commit 9cef2f0 into mainMay 14, 2026
209 of 211 checks passed
@mlubin
mlubin deleted the mlubin-patch-1 branch May 14, 2026 14:30
akifcorduk added a commit to akifcorduk/cuopt that referenced this pull request May 17, 2026
Brings in upstream's unified OpenMP threading model (PR NVIDIA#1099) and other
fixes (NVIDIA#1206 concurrent LP exception cleanup, NVIDIA#1214/NVIDIA#1216 destruction/
capture fixes) while preserving local work on the cut and clique stack.
Conflict resolution highlights:
- Drop std::future/std::async clique flow; adopt upstream's omp task +
omp_atomic_t<bool> signal_extend pattern.
- Drop modify_problem parameter from find_initial_cliques (we already
removed the code that consumed it); adapt the omp-task call site in
branch_and_bound::solve accordingly.
- Take upstream's [this, &population] capture for the root-LP CPUFJ
improvement callback; the new omp taskwait-before-destruction guarantee
makes the prior context-lifetime fix unnecessary.
- Take upstream's do_cut_pass refactor of the per-pass LP resolve loop;
move our per-pass root_lp_with_cuts publish into do_cut_pass so the
benchmark metric is still updated on early exits.
- Keep our out-of-line omp_mutex_t definitions in omp_helpers.cpp; the
enhanced omp_atomic_t with std::memory_order is taken from upstream.
chris-maes pushed a commit to chris-maes/cuopt that referenced this pull request May 18, 2026
early_fj_start is a local variable and should be captured by value since the callback outlives the local scope.
Authors:
- Miles Lubin (https://github.com/mlubin)
Approvers:
- Rajesh Gandham (https://github.com/rg20)
URL: NVIDIA#1214
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugSomething isn't workingnon-breakingIntroduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mlubin@rg20