C++23: Replace variant<T, string> with expected by elazarg · Pull Request #1074 · vbpf/prevail

elazarg · 2026-04-15T09:46:19Z

Part of #1027: replace std::variant<InstructionSeq, std::string> with std::expected<InstructionSeq, std::string>.

Signed-off-by: Elazar Gershuni <elazarg@gmail.com>

coderabbitai · 2026-04-15T09:46:35Z

📝 Walkthrough

Walkthrough

The prevail::unmarshal overloads' return type is migrated from std::variant<InstructionSeq, std::string> to std::expected<InstructionSeq, std::string>. All call sites across main, library, and tests are updated to use expected-like semantics (has_value(), error()) instead of variant operations; new Program::from_raw/disassemble helpers added to IR APIs.

Changes

Cohort / File(s)	Summary
Unmarshal API `src/ir/unmarshal.hpp`, `src/ir/unmarshal.cpp`	Return type changed from `std::variant<InstructionSeq, std::string>` to `std::expected<InstructionSeq, std::string>`; added `#include <expected>`; error branch uses `std::unexpected(...)`; parameter namespace qualification simplified.
IR public API additions `src/ir/cfg_builder.cpp`, `src/ir/program.hpp`, `src/ir/syntax.hpp`, `src/ir/marshal.hpp`	Added `Program::from_raw(...)`, `disassemble(...)`; updated `print` signature to accept `std::optional<Label>`; new includes and optional-based error propagation from unmarshal.
Printing/disassembly helpers `src/printing.cpp`, `src/main.cpp`	Added `disassemble` helper; callers updated to handle `std::expected` (`has_value()`, `error()`), use `*inst_seq` for successful InstructionSeq.
Call site updates (tests & runners) `src/test/.cpp`, `src/test/.hpp`, `src/test//.cpp`, `src/test/run_yaml.cpp`, `src/test/test_yaml.cpp`	All tests updated to treat unmarshal result as expected-like: check `has_value()`/`error()` and dereference `*inst_seq`; includes adjusted.
Build/install changes `CMakeLists.txt`	Adjusted header install directives: excluded `ir` headers from bulk install and explicitly installed selected `src/ir/*.hpp`.
Minor formatting `src/test/test_elf_loader.cpp`	Reformatted a `SECTION` invocation to single-line; no behavior changes.

Possibly related PRs

Implement checks for the --line-info flag and move program selection into read_elf #946: Edits the same unmarshal APIs and call sites; likely touches identical signatures and usage.
ISA feature support matrix and precise rejection semantics #999: Overhauls unmarshal semantics and instruction handling in src/ir/unmarshal.*; strongly related to this API change.
Use get_if instead of holds_alternative to avoid repetition #703: Changes call-site handling of unmarshalled instruction results; overlaps variant-to-value handling patterns.

Suggested labels

codex

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 29.63% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'C++23: Replace variant<T, string> with expected' clearly and specifically summarizes the main change: replacing std::variant with std::expected for error handling.
Description check	✅ Passed	The description states the PR is part of issue `#1027` and replaces std::variant<InstructionSeq, std::string> with std::expected<InstructionSeq, std::string>, which directly relates to the changeset.

_{✏️ 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 expected

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@src/ir/unmarshal.hpp`:
- Line 5: unmarshal.hpp currently exposes the C++23 header <expected> to public
APIs (via ebpf_verifier.hpp); remove the direct public dependency by introducing
a small compatibility alias (e.g., expected_compat.hpp) that conditionally uses
std::expected when __cplusplus >= 202302L and falls back to a header-only
tl::expected (or a project-local Expected<T,E>) for C++20, then replace `#include`
<expected> in unmarshal.hpp with `#include` "expected_compat.hpp" and update any
public function signatures that reference std::expected to use the compatibility
alias (e.g., Expected<T,E>) so C++20 consumers can include the installed
headers.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3fba3bbb-f72d-4130-abd2-35062a4492d4

📥 Commits

Reviewing files that changed from the base of the PR and between c0a0360 and e0da22e.

📒 Files selected for processing (10)

src/ir/unmarshal.cpp
src/ir/unmarshal.hpp
src/main.cpp
src/test/ebpf_yaml.cpp
src/test/test_elf_loader.cpp
src/test/test_failure_slice.cpp
src/test/test_marshal.cpp
src/test/test_print.cpp
src/test/test_verify.hpp
src/test/test_verify_multithreading.cpp

coderabbitai · 2026-04-15T09:54:47Z

 // SPDX-License-Identifier: MIT
 #pragma once

+#include <expected>


⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash set -euo pipefail echo "== C++ standard configuration ==" rg -n --iglob 'CMakeLists.txt' --iglob '*.cmake' 'CMAKE_CXX_STANDARD|CXX_STANDARD|cxx_std_' echo echo "== expected feature usage / compatibility shims ==" rg -n 'std::expected|<expected>|tl::expected|expected.hpp|std::unexpected'

Repository: vbpf/prevail

Length of output: 1136

🏁 Script executed:

cd examples/using_installed_package && cat -n CMakeLists.txt

Repository: vbpf/prevail

Length of output: 727

🏁 Script executed:

grep -r "unmarshal\|ir" examples/using_installed_package/ --include="*.cpp" --include="*.hpp" --include="*.h"

Repository: vbpf/prevail

Length of output: 173

🏁 Script executed:

cat -n examples/using_installed_package/main.cpp

Repository: vbpf/prevail

Length of output: 1221

🏁 Script executed:

cat -n src/prevail.hpp | head -50

Repository: vbpf/prevail

Length of output: 103

🏁 Script executed:

grep -n "unmarshal" src/prevail.hpp

Repository: vbpf/prevail

Length of output: 104

🏁 Script executed:

find . -name "prevail.hpp" -type f

Repository: vbpf/prevail

Length of output: 38

🏁 Script executed:

git ls-files | grep -E "\.hpp$" | head -20

Repository: vbpf/prevail

Length of output: 535

🏁 Script executed:

git ls-files | grep -i "prevail" | grep -E "\.(hpp|h)$"

Repository: vbpf/prevail

Length of output: 38

🏁 Script executed:

rg -n "unmarshal" --iglob "*.hpp" --iglob "*.h" src/

Repository: vbpf/prevail

Length of output: 977

🏁 Script executed:

cat -n src/ebpf_verifier.hpp

Repository: vbpf/prevail

Length of output: 372

🏁 Script executed:

grep -rn "public\|install" CMakeLists.txt | head -30

Repository: vbpf/prevail

Length of output: 577

🏁 Script executed:

cat -n CMakeLists.txt | sed -n '240,265p'

Repository: vbpf/prevail

Length of output: 1084

🏁 Script executed:

cat -n src/ebpf_verifier.hpp

Repository: vbpf/prevail

Length of output: 372

C++23 features in public headers break C++20 consumer builds.

The project is configured for C++23 (CMakeLists.txt:33), but unmarshal.hpp exposes std::expected (C++23 only) in the public API via ebpf_verifier.hpp. This prevents C++20-configured consumers like examples/using_installed_package from using the installed library headers. Either restrict C++23 features to implementation-only files, provide a C++20-compatible compatibility layer, or document this as a breaking requirement for library consumers.

🧰 Tools

🪛 Clang (14.0.6)

[error] 5-5: 'expected' file not found

(clang-diagnostic-error)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@src/ir/unmarshal.hpp` at line 5, unmarshal.hpp currently exposes the C++23 header <expected> to public APIs (via ebpf_verifier.hpp); remove the direct public dependency by introducing a small compatibility alias (e.g., expected_compat.hpp) that conditionally uses std::expected when __cplusplus >= 202302L and falls back to a header-only tl::expected (or a project-local Expected<T,E>) for C++20, then replace `#include` <expected> in unmarshal.hpp with `#include` "expected_compat.hpp" and update any public function signatures that reference std::expected to use the compatibility alias (e.g., Expected<T,E>) so C++20 consumers can include the installed headers.

@Alan-Jowett do we need unmarshal.hpp as part of the API?

Yes, we currently call prevail::unmarshal() directly in 4 places across verifier_service.cpp and Verifier.cpp, and ebpf_verifier.hpp includes unmarshal.hpp. However, in all cases we immediately pass the InstructionSeq to the next step (Program::from_sequence, print, or analyze) — we never use InstructionSeq on its own.

So if prevail provided higher-level APIs that accept RawProgram directly (e.g., Program::from_raw(RawProgram, notes, options) that internalizes the unmarshal step, and a disassemble-to-stream function), we could stop depending on unmarshal.hpp and InstructionSeq entirely. That would also resolve the C++23 public API concern since std::expected would stay internal.

Our project currently builds with C++20, so exposing std::expected in public headers would require us to bump our standard.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Something like this?
https://github.com/vbpf/prevail/pull/1074/changes#diff-6714101bca0d9472baf09f9c8eaf5708d12f6fa8f5dc3160e52f319eb2e32310

/// Build a Program directly from a raw (ELF-bytecode) representation. /// On failure returns std::nullopt and appends the error message to `notes` /// (the same vector that collects unmarshal warnings). static std::optional<Program> from_raw(const RawProgram& raw_prog, std::vector<std::vector<std::string>>& notes, const ebpf_verifier_options_t& options);

/// Write a textual disassembly of `raw_prog` to `out`. On failure writes the /// error message to `out` and returns false. bool disassemble(const RawProgram& raw_prog, const ebpf_verifier_options_t& options, std::ostream& out, const std::optional<Label>& label_to_print = {}, bool print_line_info = false);

Signed-off-by: Elazar Gershuni <elazarg@gmail.com>

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@src/printing.cpp`:
- Around line 730-739: The error message in disassemble (function disassemble,
variable inst_seq) uses the preposition "at" which misleadingly implies a
location; update the error output to use "unmarshaling error: " (colon) and
include inst_seq.error() so the full error text is preserved—i.e., change the
out << "unmarshaling error at " << inst_seq.error(); to use a clearer separator
like ":" while keeping the rest of the logic intact.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: fb4fdb84-0628-45ff-8a99-099c6e380036

📥 Commits

Reviewing files that changed from the base of the PR and between e0da22e and 4ed60f3.

📒 Files selected for processing (14)

CMakeLists.txt
docs/architecture.md
src/ebpf_verifier.hpp
src/ir/cfg_builder.cpp
src/ir/program.hpp
src/ir/syntax.hpp
src/main.cpp
src/printing.cpp
src/test/ebpf_yaml.cpp
src/test/run_yaml.cpp
src/test/test_failure_slice.cpp
src/test/test_marshal.cpp
src/test/test_verify.hpp
src/test/test_yaml.cpp

💤 Files with no reviewable changes (2)

src/test/test_yaml.cpp
src/ebpf_verifier.hpp

replace variant<T, string> with expected

e0da22e

Signed-off-by: Elazar Gershuni <elazarg@gmail.com>

coderabbitai bot reviewed Apr 15, 2026

View reviewed changes

vbpf deleted a comment from coderabbitai bot Apr 15, 2026

avoid leaking C++23 to public API

4ed60f3

Signed-off-by: Elazar Gershuni <elazarg@gmail.com>

coderabbitai bot reviewed Apr 15, 2026

View reviewed changes

Comment thread src/printing.cpp

This was referenced Apr 15, 2026

Define a project-level error-handling policy #1079

Open

Populate the stable public verifier API facade #1083

Open

Split runtime semantics from presentation and orchestration options #1085

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++23: Replace variant<T, string> with expected#1074

C++23: Replace variant<T, string> with expected#1074
elazarg wants to merge 2 commits intomainfrom
expected

elazarg commented Apr 15, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Apr 15, 2026 •

edited

Loading

Walkthrough

Changes

Possibly related PRs

Suggested labels

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Apr 15, 2026 •

edited

Loading

Uh oh!

elazarg Apr 15, 2026

Uh oh!

Alan-Jowett Apr 15, 2026

Uh oh!

coderabbitai bot Apr 15, 2026

Uh oh!

elazarg Apr 15, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

elazarg commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Possibly related PRs

Suggested labels

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elazarg Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Alan-Jowett Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

elazarg Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

elazarg commented Apr 15, 2026 •

edited

Loading

coderabbitai bot commented Apr 15, 2026 •

edited

Loading

coderabbitai bot Apr 15, 2026 •

edited

Loading