Skip to content

refactor(core): C++23 Wave 5 — cpu, ref, thread_locale (ADR-0735)#58

Open
lusoris wants to merge 2 commits into
masterfrom
feat/cpp23-wave5-bundle-20260528
Open

refactor(core): C++23 Wave 5 — cpu, ref, thread_locale (ADR-0735)#58
lusoris wants to merge 2 commits into
masterfrom
feat/cpp23-wave5-bundle-20260528

Conversation

@lusoris
Copy link
Copy Markdown
Contributor

@lusoris lusoris commented May 28, 2026

Summary

  • cpu.c → cpu.cpp: std::atomic<unsigned> for CPU-flag globals — eliminates potential init-time data-race; memory_order_relaxed on the hot vmaf_get_cpu_flags() read path.
  • ref.c → ref.cpp: std::make_unique<VmafRef>() replaces malloc+memset+atomic_init; leak-safe error paths; vmaf_ref_close uses delete; extern "C" guards added to ref.h.
  • thread_locale.c → thread_locale.cpp: VmafThreadLocaleState destructor encapsulates platform teardown (POSIX / Win32); vmaf_thread_locale_pop is a unique_ptr one-liner; std::array<char,256> on Windows.

Each file compiled as an isolated static_library with override_options: ['cpp_std=c++23'] (ADR-0708 pattern). Public C ABI preserved unchanged.

Test plan

# Build host-side CPU-only
cd core && meson setup ../build-wave5 -Denable_cuda=false -Denable_sycl=false -Denable_asm=true
ninja -C ../build-wave5
meson test -C ../build-wave5 --suite=fast
# Expected: 49/49 PASS

Checklist (ADR-0108 six deliverables)

  • Research digest: no digest needed: bulk Wave 5 continues ADR-0708 playbook
  • Decision matrix: docs/adr/0735-cpp23-wave5-bundle.md § Alternatives considered
  • AGENTS.md invariant: core/AGENTS.md — C++23 migration surface table updated
  • Reproducer: meson commands above
  • Changelog fragment: changelog.d/changed/cpp23-wave5.md
  • Rebase notes: docs/rebase-notes.md — ADR-0735 entry

Per-surface docs

no user-discoverable surface change — internal refactor

docs/state.md

no bug opened/closed/ruled-out — refactor only

ffmpeg-patches

no public C-API change — internal TUs only

🤖 Generated with Claude Code

@lusoris lusoris marked this pull request as ready for review May 28, 2026 17:45
@lusoris lusoris enabled auto-merge (squash) May 28, 2026 17:45
Convert three small core/src/*.c files to C++23, continuing the ADR-0708
migration playbook (isolated static_library per TU, cpp_std=c++23 scoped):

- cpu.cpp: std::atomic<unsigned> for CPU-flag state, eliminating a
  potential data-race on multi-threaded init; memory_order_relaxed on
  the hot vmaf_get_cpu_flags() read path preserves zero overhead.
- ref.cpp: std::make_unique<VmafRef>() replaces malloc+memset+atomic_init;
  early-return paths are leak-safe; vmaf_ref_close uses delete (matching
  operator new). extern "C" guards added to ref.h.
- thread_locale.cpp: VmafThreadLocaleState destructor encapsulates
  platform teardown (POSIX uselocale/freelocale, Win32
  _configthreadlocale); vmaf_thread_locale_pop reduced to a unique_ptr
  adopt-and-destroy one-liner; std::array<char,256> on Windows.

Public C ABI preserved. Build: 726/726. Tests: 49/49 fast suite pass.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lusoris
Copy link
Copy Markdown
Contributor Author

lusoris commented May 28, 2026

CRITICAL finding from adversarial review: ref.cpp vmaf_ref_init now allocates VmafRef via std::make_unique (operator new), but vmaf_ref_close uses delete. Any C caller that calls free(ref_ptr) directly on a VmafRef* (a pre-existing pattern from when the allocator was malloc) will corrupt the heap — free() on a operator new pointer is UB. This is a silent ABI contract change invisible from the ref.h signature. Fix: add a prominent comment to ref.h that vmaf_ref_close is the ONLY valid deallocator, and audit all C callers (picture_pool.c, framesync.c, etc.) for any direct free(VmafRef*) calls. See docs/research/cpp23-wave-adversarial-review-20260528.md finding #18.

…in vmaf_ref_close (adversarial review PR #78)

make_unique uses operator new; vmaf_ref_close (C ABI) must use free() to
match the legacy allocator contract and keep ref.c (test harness) and
ref.cpp consistent. Switch to malloc+placement-new and document in ref.h
that vmaf_ref_close is the sole valid deallocator.
Fixes the CRITICAL allocator-mismatch finding in adversarial review PR #78.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lusoris
Copy link
Copy Markdown
Contributor Author

lusoris commented May 28, 2026

Fix for CRITICAL adversarial review finding in PR #78 pushed as 371a0f3. See PR #78 digest for context.

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.

1 participant