_⚠️ Potential issue_ | _🟡 Minor_

[vtz]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all callers of get_value()/move_value() to confirm they all pre-check is_error()/is_success().
rg -nP -C3 '\b(get_value|move_value)\s*\(\s*\)' --type=cpp --type=c

Repository: vtz/opensomeip

Length of output: 32716

---

In-tree usage is correctly guarded; but confirm public API contract for external consumers.

All verified in-tree calls to get_value() and move_value() are properly guarded with is_error() checks, confirming they cannot trigger std::abort() in normal operation. However, the switch from throwing std::bad_optional_access to calling std::abort() is a breaking change if this class is exposed as public API — downstream consumers may have relied on exception handling for recovery. In safety-critical contexts (SOME/IP stack), consider whether unconditional std::abort() is the right choice versus:

• Preserving exception behavior (maintaining API compatibility)
• Debug-only assert (catches errors early during development)
• std::terminate() (clearer intent than abort for fatal errors)

Confirm this class's API contract and whether public consumers exist before finalizing this change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@include/serialization/serializer.h` around lines 80 - 95, The change from
throwing std::bad_optional_access to calling std::abort() in get_value() and
move_value() is a breaking API change for external consumers; revert to the
previous exception behavior (throw std::bad_optional_access when value_ is
empty) or, if abort-like behavior is desired only in debug builds, replace the
unconditional std::abort() with a debug-only assertion (e.g.,
assert(value_.has_value())) and throw std::bad_optional_access in release
builds; update the public API contract/comments on get_value() and move_value()
to document the chosen behavior so downstream users know whether they must
handle exceptions or if the call is fatal.

Originally posted by @coderabbitai[bot] in #234 (comment)

[vtz]

Closing — verified this is not a real issue for this codebase.

Analysis:

1. All in-tree callers are properly guarded. Every call to get_value()/move_value() across both production code (serializer.cpp, serializer.h templates) and tests (test_serialization.cpp) is preceded by an is_success() or is_error() check. The std::abort() path is unreachable in normal operation.

2. The project explicitly targets -fno-exceptions builds. The ARM cross-compile toolchain (cmake/toolchains/arm-none-eabi-gcc.cmake) sets -fno-exceptions -fno-rtti. The previous std::optional::value() call, which throws std::bad_optional_access, would have invoked std::terminate() on those platforms anyway — making the old behavior functionally equivalent to std::abort() but less explicit.

3. std::abort() is the correct choice for a safety-critical SOME/IP stack. In ASIL-B contexts, a precondition violation (calling get_value() on an error result) should fail fast, not attempt exception-based recovery. Preserving exceptions would weaken the safety contract.

4. DeserializationResult is an internal Result-type class, not a general-purpose public API. The docstrings explicitly state get_value() is "only valid if is_success() returns true" — calling it without checking is a programming error, not a recoverable condition. The class was designed specifically to avoid exceptions via the Result pattern.

The change from .value() to explicit has_value() check + std::abort() was a deliberate clang-tidy remediation that made the fail-fast contract portable and explicit. No fix needed.