refactor(core): C++23 pilot Wave 1 — opt.c → opt.cpp (ADR-0721)

changelog.d/changed/cpp23-opt.md

@@ -0,0 +1,12 @@
+# C++23 Wave 1: `opt.c` → `opt.cpp` (ADR-0721)
+
+`core/src/opt.c` converted to `core/src/opt.cpp` using C++23 `std::optional<T>` for
+the internal parse helpers (`parse_bool`, `parse_int`, `parse_double`). The public C
+ABI (`vmaf_option_set`) is unchanged: same signature, same return contract, same
+case-sensitive string comparison behaviour. `opt.h` now carries `extern "C"` guards so
+all C callers compile without modification. Build: `opt_cpp23_lib` isolated static
+library compiled at `cpp_std=c++23`, following the ADR-0708 playbook.
+
+Also: removes stale `test_ansnr_simd` test registration from `core/test/meson.build`
+(the source files were deleted in ADR-0720 / PR #38 but the meson stanza was not
+cleaned up, causing `meson setup` to fail).

core/src/opt.cpp

@@ -0,0 +1,158 @@
+/**
+ *
+ *  Copyright 2016-2026 Netflix, Inc.
+ *
+ *     Licensed under the BSD+Patent License (the "License");
+ *     you may not use this file except in compliance with the License.
+ *     You may obtain a copy of the License at
+ *
+ *         https://opensource.org/licenses/BSDplusPatent
+ *
+ *     Unless required by applicable law or agreed to in writing, software
+ *     distributed under the License is distributed on an "AS IS" BASIS,
+ *     WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *     See the License for the specific language governing permissions and
+ *     limitations under the License.
+ *
+ */
+
+#include <cerrno>
+#include <climits>
+#include <cmath>
+#include <cstring>
+#include <cstdlib>
+#include <optional>
+#include <string>
+#include <string_view>
+
+#include "opt.h"
+
+// ---------------------------------------------------------------------------
+// Internal helpers returning std::optional<T> — parse failure → nullopt.
+// These never touch the C errno state beyond what strtol/strtod set themselves.
+// The C ABI boundary (vmaf_option_set) converts nullopt → -EINVAL.
+// ---------------------------------------------------------------------------
+
+static std::optional<bool> parse_bool(std::string_view sv) noexcept
+{
+    if (sv == "true")
+        return true;
+    if (sv == "false")
+        return false;
+    return std::nullopt;
+}
+
+static std::optional<int> parse_int(std::string_view sv, int min_val, int max_val) noexcept
+{
+    if (sv.empty())
+        return std::nullopt;
+
+    /* Copy sv into a NUL-terminated std::string so strtol is never given a
+     * non-NUL-terminated buffer regardless of how the caller constructed sv.
+     * Eliminates the UB class identified in adversarial review PR #78. */
+    const std::string s_int(sv);
+    char *end = nullptr;
+    errno = 0;
+    const long n = std::strtol(s_int.c_str(), &end, 10);
+    if (end == s_int.c_str() || *end != '\0')
+        return std::nullopt;
+    if (errno == ERANGE)
+        return std::nullopt;
+    if (n < static_cast<long>(min_val) || n > static_cast<long>(max_val))
+        return std::nullopt;
+    return static_cast<int>(n);
+}
+
+static std::optional<double> parse_double(std::string_view sv, double min_val,
+                                          double max_val) noexcept
+{
+    if (sv.empty())
+        return std::nullopt;
+
+    /* Copy sv into a NUL-terminated std::string so strtod is never given a
+     * non-NUL-terminated buffer regardless of how the caller constructed sv.
+     * Eliminates the UB class identified in adversarial review PR #78. */
+    const std::string s_dbl(sv);
+    char *end = nullptr;
+    errno = 0;
+    const double n = std::strtod(s_dbl.c_str(), &end);
+    if (end == s_dbl.c_str() || *end != '\0')
+        return std::nullopt;
+    if (errno == ERANGE)
+        return std::nullopt;
+    /* NaN bypasses ordered comparisons (NaN < x and NaN > x are both false),
+     * so reject it explicitly before the bounds check. Infinity is already
+     * rejected when the bound is finite (Inf > max is true), but NaN is not.
+     * T-ROUND8-OPT-NAN-BYPASS / CWE-704. */
+    if (std::isnan(n))
+        return std::nullopt;
+    if (n < min_val || n > max_val)
+        return std::nullopt;
+    return n;
+}
+
+// ---------------------------------------------------------------------------
+// C ABI entry point — identity preserved exactly (same signature, same errno
+// / return-code contract).  The [[nodiscard]] annotation is advisory for C++
+// callers; the C callers in feature_extractor.c are unaffected.
+// ---------------------------------------------------------------------------
+
+[[nodiscard]] int vmaf_option_set(const VmafOption *opt, void *obj, const char *val)
+{
+    if (!obj || !opt)
+        return -EINVAL;
+
+    uint8_t *base = static_cast<uint8_t *>(obj) + opt->offset;
+
+    switch (opt->type) {
+    case VMAF_OPT_TYPE_BOOL: {
+        bool *dst = reinterpret_cast<bool *>(base);
+        *dst = opt->default_val.b;
+        if (!val)
+            return 0;
+        auto result = parse_bool(val);
+        if (!result)
+            return -EINVAL;
+        *dst = *result;
+        return 0;
+    }
+    case VMAF_OPT_TYPE_INT: {
+        int *dst = reinterpret_cast<int *>(base);
+        *dst = opt->default_val.i;
+        if (!val)
+            return 0;
+        auto result = parse_int(val, static_cast<int>(opt->min), static_cast<int>(opt->max));
+        if (!result)
+            return -EINVAL;
+        *dst = *result;
+        return 0;
+    }
+    case VMAF_OPT_TYPE_DOUBLE: {
+        double *dst = reinterpret_cast<double *>(base);
+        *dst = opt->default_val.d;
+        if (!val)
+            return 0;
+        auto result = parse_double(val, opt->min, opt->max);
+        if (!result)
+            return -EINVAL;
+        *dst = *result;
+        return 0;
+    }
+    case VMAF_OPT_TYPE_STRING: {
+        char **dst = reinterpret_cast<char **>(base);
+        *dst = opt->default_val.s;
+        if (!val)
+            return 0;
+        /* String options store a borrowed pointer — lifetime owned by the
+         * caller; no allocation here, matching the original C behaviour. */
+        *dst = const_cast<char *>(val); // NOLINT(cppcoreguidelines-pro-type-const-cast)
+        // ADR-0721: the public VmafOption API exposes `char *` (not `const char *`)
+        // for string values. Removing the const_cast would require a public ABI
+        // change (VmafOption.default_val.s type). Preserved verbatim for ABI
+        // stability; the original opt.c performed the identical implicit cast.
+        return 0;
+    }
+    default:
+        return -EINVAL;
+    }
+}

docs/adr/0721-cpp23-pilot-opt.md

@@ -0,0 +1,77 @@
+# ADR-0721: C++23 Pilot Wave 1 — `opt.c` conversion
+
+- **Status**: Accepted
+- **Date**: 2026-05-28
+- **Deciders**: lusoris
+- **Tags**: build, c++, cpp23, refactor, internals, fork-local, vmafx-rebrand
+
+## Context
+
+Following the ADR-0708 pilot (`metadata_handler.c → .cpp`, C++20), this ADR records
+the Wave 1 conversion of `core/src/opt.c` to C++23. The `opt.c` file implements
+option parsing for feature extractor parameters: it converts string values (`"true"`,
+`"123"`, `"0.5"`) to typed C fields via `vmaf_option_set`. Research-0732 assigned
+it ROI 1.5 — moderate benefit from C++ idioms, low risk.
+
+The original implementation had four static helpers returning bare `int` (0 or
+`-EINVAL`), with `strtol`/`strtod` error paths scattered across the function body.
+C++23 `std::optional<T>` cleanly models "parse succeeded → value; failed → nothing"
+without any loss of performance (small, stack-only, inlinable).
+
+The public C ABI (`vmaf_option_set`) is identical: same name, same parameter types,
+same return contract (`0` on success, `-EINVAL` on error).
+
+## Decision
+
+We will convert `core/src/opt.c` → `core/src/opt.cpp` using C++23 `std::optional`
+for the internal parse helpers, with the following constraints:
+
+1. `opt.h` gains `extern "C" { ... }` guards so all existing C callers
+   (`feature_extractor.c` and test TUs) continue to compile and link unchanged.
+2. The `meson.build` entry for the file is replaced by a dedicated
+   `opt_cpp23_lib` static library using `override_options : ['cpp_std=c++23']`,
+   following the ADR-0708 pattern for `metadata_handler_cpp20_lib`.
+3. All test executables that previously included `'../src/opt.c'` in their
+   sources list now pull in `opt_cpp23_lib.extract_all_objects(recursive: true)`
+   from the objects list — the same mechanism used for `metadata_handler.cpp`
+   tests in ADR-0708.
+4. Internal helpers (`parse_bool`, `parse_int`, `parse_double`) return
+   `std::optional<T>`; nullopt maps to `-EINVAL` at the C ABI boundary.
+5. `[[nodiscard]]` is added to `vmaf_option_set` for C++ callers; C callers are
+   unaffected.
+6. Case-sensitivity and locale behaviour of string comparisons are preserved
+   exactly: `strcmp("true")` / `strcmp("false")` — no locale-aware alternatives.
+7. Fast-suite test gate must pass post-conversion (50/50 tests).
+
+## Alternatives considered
+
+| Option | Pros | Cons | Why not chosen |
+|---|---|---|---|
+| `std::expected<T, int>` instead of `std::optional<T>` | Carries the error code directly; idiomatic C++23 | libc++ 17+ required; host g++ 12/13 ships only draft `<expected>` or none; would break the CI toolchain | Deferred to Wave 2 / post-toolchain-bump |
+| Keep `const char *` params, no `std::string_view` | Zero diff for callers | `string_view` enables safer bounds-checked operations without runtime cost | `string_view` adopted for all read-only string params in internal helpers |
+| Nullable pointer `T *out` instead of `std::optional<T>` | C-compatible return style | Heap or stack pointer; more verbose at call site; no improvement over original | `std::optional` is the idiomatic C++ form and stack-allocated |
+| Bump project `cpp_std=c++23` globally | Simpler build | Risks breaking SYCL / CUDA TUs not validated at C++23 yet | Per-target override is safe now; global bump deferred per ADR-0708 rationale |
+| Do not convert — keep C23 | No new compiler dependency | C23 has `ckd_*`, `typeof`, but no `optional` / RAII | Cannot meet the `std::optional` modelling goal with C alone |
+
+## Consequences
+
+- **Positive**: `parse_int` / `parse_double` path-of-failure is unambiguous at
+  call sites — `!result` rather than inspecting errno + return value. Future
+  contributors extending the type switch have a clear pattern to follow.
+- **Negative**: `opt.cpp` now requires a C++23-capable compiler on the build
+  host. This was already required by `svm.cpp` (C++17) and all SYCL TUs
+  (C++17/20), so no new toolchain dependency is introduced.
+- **Neutral / follow-ups**:
+  - Wave 2 candidates: `log.c` (ROI 3.0), `mem.c` (ROI 2.0).
+  - `dict.c` (Wave 3, ROI 0.83) requires `std::expected` → deferred to
+    post-global `cpp_std=c++23` bump.
+  - The ansnr SIMD test stub removal (test/meson.build cleanup) was bundled
+    into this PR because the deleted source files were causing meson setup to
+    fail — a pre-existing gap from the ADR-0720 ansnr drop (PR #38).
+
+## References
+
+- ADR-0708 (`docs/adr/0708-vmafx-cpp23-internals-pilot.md`) — pilot and recipe.
+- Research-0732 (`docs/research/0732-vmafx-cpp23-internals-migration-plan.md`) — ROI ranking.
+- ADR-0692 (`docs/adr/0692-vmafx-c23-bump.md`) — C23 bump; `cpp_std` override pattern.
+- req: "Internal implementation moves `.c` to `.cpp` where C++23 features help: `std::optional` to replace raw int-return parse helpers."

docs/adr/_index_fragments/0721-cpp23-pilot-opt.md

@@ -0,0 +1 @@
+| [ADR-0721](0721-cpp23-pilot-opt.md) | C++23 Wave 1 pilot: convert `opt.c` → `opt.cpp` using `std::optional<T>` for parse helpers; `extern "C"` guards; `[[nodiscard]]` on public entry point; isolated `opt_cpp23_lib` static-lib pattern (ADR-0708 playbook). | Accepted | 2026-05-28 | build, c++, cpp23, refactor, internals, fork-local, vmafx-rebrand |