benchmarks/pch_chain/pch_chain_benchmark.cpp (6)

214-221: Magic number 3 for diagnostic level filtering.

The comparison static_cast<int>(diag.id.level) >= 3 uses a magic number. If this represents error-level diagnostics, consider using a named constant or enum comparison for clarity.

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 214 - 221, Replace
the magic number "3" used to filter diagnostics by level with a named constant
or enum comparison: locate the loop over unit.diagnostics() and change the
conditional that reads static_cast<int>(diag.id.level) >= 3 to compare
diag.id.level against the proper DiagnosticLevel enum value (e.g.,
DiagnosticLevel::Error or a named constant like DIAGNOSTIC_LEVEL_ERROR) so the
intent is clear and type-safe.

---

475-478: Consider structured cleanup over goto.

Using goto cleanup is valid C++ but generally discouraged. A scope guard or early-return with cleanup in a destructor would be more idiomatic. However, for a benchmark utility this is acceptable.

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 475 - 478, Replace
the goto-based cleanup in the if(!r.success) branch with RAII or an explicit
early return: remove the goto cleanup and ensure all resources cleaned by
introducing a scope-guard/cleanup object (or wrapping resources in a class with
a destructor) that handles the cleanup when it goes out of scope, then log the
failure using the existing std::println call for r.success failure in the loop
(the branch checking r.success and using headers[i]) and either return or break
to exit the function/loop so explicit cleanup via goto isn't needed.

---

145-153: Static storage for arguments is fragile and inconsistent with verify_pch.

The static std::vector<std::string> storage pattern returns pointers that become invalid if make_pch_args is called again before the previous args are consumed. While this works in the current single-threaded usage, it differs from the safer local-storage approach used in verify_pch (lines 242-247).

Consider aligning with the local storage pattern for consistency and safety:

♻️ Suggested refactor using output parameter

-static std::vector<const char*> make_pch_args(const std::string& file,
-                                              const std::string& resource_dir) {
-    static std::vector<std::string> storage;
-    storage = {"clang++", "-std=c++20", "-resource-dir", resource_dir, "-x", "c++-header", file};
-    std::vector<const char*> args;
-    for(auto& s: storage)
-        args.push_back(s.c_str());
-    return args;
+static std::vector<const char*> make_pch_args(const std::string& file,
+                                              const std::string& resource_dir,
+                                              std::vector<std::string>& storage) {
+    storage = {"clang++", "-std=c++20", "-resource-dir", resource_dir, "-x", "c++-header", file};
+    std::vector<const char*> args;
+    for(auto& s: storage)
+        args.push_back(s.c_str());
+    return args;
 }

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 145 - 153, The
function make_pch_args currently uses a static std::vector<std::string> storage
which yields c_str() pointers that can be invalidated on subsequent calls;
change it to avoid static storage by either (A) making storage a caller-owned
output parameter (e.g., add std::vector<std::string>& storage_out) and populate
it inside make_pch_args, then build and return the std::vector<const char*> from
storage_out, or (B) return both storage and args (e.g., a struct or pair) so the
caller owns the string storage; update call sites that used make_pch_args
accordingly and mirror the local-storage approach used in verify_pch to ensure
the returned const char* pointers remain valid.

---

547-552: Input validation is minimal but acceptable for benchmark CLI.

std::atoi doesn't handle invalid input (returns 0 for non-numeric). Negative values for --runs or --chain-length could cause issues. For a developer benchmark tool, this is acceptable, but be aware during usage.

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 547 - 552, The CLI
currently parses --runs and --chain-length using std::atoi which returns 0 on
invalid input and allows negative values; update parsing for argv entries for
"runs" and "chain_length" (the variables runs and chain_length, and the arg
checks for "--runs" and "--chain-length") to use a safer conversion (e.g.,
std::stoi inside a try/catch or manual digit check) and validate the result is
non-negative; then clamp chain_length against ALL_HEADERS.size() as already done
and ensure runs is at least 1 (or another sensible minimum) to avoid downstream
errors.

---

231-233: Virtual path /tmp/pch-verify.cpp should also use portable path.

While the file is memory-remapped, using a consistent cross-platform temporary path pattern would improve Windows compatibility.

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 231 - 233, The
hard-coded VERIFY_FILE path is not portable; change VERIFY_FILE from a
compile-time constexpr to a runtime string constructed using the platform temp
directory (e.g., std::filesystem::temp_directory_path()) and a unique filename
(e.g., std::filesystem::unique_path or a timestamp/UUID) before the
memory-remap/create step so the code uses a cross-platform temporary path; keep
VERIFY_CODE as-is and update any usages that assume VERIFY_FILE is a constexpr
to use the new runtime std::string (references: VERIFY_FILE and VERIFY_CODE in
the pch verification/memory-remap logic).

---

179-179: Hardcoded /tmp path limits Windows compatibility.

cp.directory = "/tmp" won't exist on Windows. Consider using a platform-agnostic temporary directory:

♻️ Suggested fix for cross-platform compatibility

-    cp.directory = "/tmp";
+    llvm::SmallString<128> tmp_dir;
+    llvm::sys::path::system_temp_directory(true, tmp_dir);
+    cp.directory = std::string(tmp_dir);

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` at line 179, The assignment
cp.directory = "/tmp" hardcodes a Unix path and breaks on Windows; change it to
use a platform-agnostic temp path (e.g., use
std::filesystem::temp_directory_path() or an equivalent helper) and assign that
result to cp.directory (falling back to a sensible default if
temp_directory_path() throws); update any code using cp.directory to handle an
empty/fallback value if needed.

Fix all unresolved CodeRabbit comments on this PR:

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

benchmarks/pch_chain/pch_chain_benchmark.cpp (2)

214-221: Consider replacing magic number with named constant.

The comparison static_cast<int>(diag.id.level) >= 3 uses a magic number. If DiagnosticLevel has named constants (e.g., DiagnosticLevel::Error), using them would improve readability.

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 214 - 221, The
loop is using a magic number (3) in the comparison
static_cast<int>(diag.id.level) >= 3; replace that with the appropriate named
enum constant (e.g., DiagnosticLevel::Error) to improve readability: compare
diag.id.level against DiagnosticLevel::Error (or
static_cast<int>(DiagnosticLevel::Error) if you must keep the int cast),
updating the condition in the loop that builds the errors string (the
diagnostics iteration over unit.diagnostics() and the diag.id.level check).

---

707-734: Consider handling symlinked or relocated binaries.

The resource directory discovery assumes the binary is at <build>/bin/pch_chain_benchmark with resources at <build>/lib/clang/<version>. If the binary is symlinked or installed elsewhere, this heuristic may fail silently until the final check at line 736.

The fallback glob search (lines 719-732) is a good mitigation, but it could be documented or a more explicit error message could indicate what paths were searched.

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 707 - 734, The
resource discovery block that computes resource_dir from argv[0] should resolve
symlinks and record searched paths: call llvm::sys::fs::real_path on bin_path
(or otherwise resolve argv[0]) before computing bin_dir/build_dir so
symlinked/relocated binaries map to their real install paths, and collect the
attempted candidate paths (e.g., rd, lib_dir + each entry p, and candidate) into
a vector for clearer diagnostics; update the final failure/error message to
include those searched paths. Modify the block that creates
bin_path/bin_dir/build_dir and the fallback loop (references: bin_path, bin_dir,
build_dir, rd, lib_dir, candidate) to first call
llvm::sys::fs::real_path(bin_path) and to append each tested path to a list used
in the error/log output when resource_dir remains empty.

Fix all unresolved CodeRabbit comments on this PR:

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

benchmarks/pch_chain/pch_chain_benchmark.cpp (3)

698-702: Redundant make_preamble calls.

preamble (Line 698) and mono_preamble (Line 702) are computed identically. Consider reusing one variable.

♻️ Suggested simplification

-    auto preamble = make_preamble(headers, count);
-    auto preamble_bound = static_cast<std::uint32_t>(preamble.size());
-
-    // --- Build monolithic PCH ---
-    std::string mono_preamble = make_preamble(headers, count);
+    std::string mono_preamble = make_preamble(headers, count);
+    auto preamble_bound = static_cast<std::uint32_t>(mono_preamble.size());
+
+    // --- Build monolithic PCH ---

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 698 - 702, The
code calls make_preamble(headers, count) twice producing both preamble and
mono_preamble; remove the redundant call by reusing the existing preamble
variable (e.g., set mono_preamble to preamble or std::move(preamble) if the
original is no longer needed) and eliminate the second make_preamble invocation
so only one preamble is constructed; update any uses that assumed separate
instances accordingly (references: preamble, mono_preamble, make_preamble).

---

836-839: Hardcoded LLVM version may require updates.

The path lib/clang/21 is hardcoded for LLVM 21.x. While the fallback search (Lines 841-854) provides resilience, consider using the fallback as the primary approach to avoid needing code changes on LLVM upgrades.

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 836 - 839, The
code currently checks for a hardcoded "lib/clang/21" path (variables and
symbols: rd, build_dir, resource_dir, llvm::SmallString,
llvm::sys::path::append, llvm::sys::fs::exists); change this to discover the
clang version dynamically by listing the build_dir/lib/clang directory,
selecting the best/ highest-version subdirectory (or the first valid one) and
using that path for resource_dir; keep the existing fallback search logic as
backup if no clang subdirectory is found.

---

789-793: Consider defensive check for ratio calculation.

Similar to the speedup calculation issue flagged previously, chain_med / mono_med could divide by zero if mono_med is 0. While less likely in practice (monolithic timing should always be positive), a guard would be consistent with fixing the other division.

🛡️ Suggested defensive guard

         if(!mono_times.empty() && !chain_times.empty()) {
             double mono_med = mono_times[mono_times.size() / 2];
             double chain_med = chain_times[chain_times.size() / 2];
-            double ratio = chain_med / mono_med;
-            std::println("  Ratio (chained/mono): {:.2f}x", ratio);
+            if(mono_med > 0)
+                std::println("  Ratio (chained/mono): {:.2f}x", chain_med / mono_med);
+            else
+                std::println("  Ratio: N/A (mono time is zero)");
         } else if(chain_times.empty()) {

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 789 - 793, The
ratio calculation can divide by zero when mono_med is 0; update the block that
computes mono_med, chain_med and ratio (using mono_times, chain_times, mono_med,
chain_med, ratio and the std::println call) to defensively check mono_med (e.g.,
fabs(mono_med) > epsilon or mono_med != 0) before computing chain_med/mono_med
and handle the zero case by printing a safe placeholder like "N/A" or "inf"
instead of performing the division; ensure the check is applied in the same
scope where ratio is computed so std::println never receives a result from
dividing by zero.

benchmarks/pch_chain/pch_chain_benchmark.cpp (5)

1046-1048: ⚠️ Potential issue | 🟡 Minor

Unsigned underflow if chain_length is 0.

bench_incremental(ALL_HEADERS, chain_length - 1, ...) will underflow to a very large value if chain_length is 0, since chain_length is std::size_t. This should be guarded after argument parsing.

🛡️ Suggested guard before benchmark calls

+    if(chain_length == 0) {
+        std::println(stderr, "Error: chain_length must be at least 1");
+        return 1;
+    }
+
     bench_monolithic(ALL_HEADERS, chain_length, runs, resource_dir);
     bench_chained(ALL_HEADERS, chain_length, runs, resource_dir);
     bench_incremental(ALL_HEADERS, chain_length - 1, runs, resource_dir);

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 1046 - 1048, The
call bench_incremental(ALL_HEADERS, chain_length - 1, ...) can underflow because
chain_length is a std::size_t; after argument parsing add a guard that only
calls bench_incremental when chain_length > 0 (or handle the zero case
explicitly), e.g. check chain_length and skip or adjust the argument instead of
subtracting unconditionally from chain_length in the bench_incremental(...)
invocation.

---

994-999: ⚠️ Potential issue | 🟡 Minor

Validate that runs and chain_length are positive.

std::atoi returns 0 for non-numeric input, and negative values are silently accepted. Either would cause benchmark loops to misbehave or, for chain_length, cause unsigned underflow at line 1048.

🛡️ Suggested validation

     for(int i = 1; i < argc; ++i) {
         std::string arg = argv[i];
         if(arg == "--runs" && i + 1 < argc) {
             runs = std::atoi(argv[++i]);
+            if(runs <= 0) {
+                std::println(stderr, "Error: --runs must be a positive integer");
+                return 1;
+            }
         } else if(arg == "--chain-length" && i + 1 < argc) {
             chain_length = std::atoi(argv[++i]);
+            if(chain_length == 0) {
+                std::println(stderr, "Error: --chain-length must be at least 1");
+                return 1;
+            }
             if(chain_length > ALL_HEADERS.size())
                 chain_length = ALL_HEADERS.size();

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 994 - 999,
Validate that parsed values for runs and chain_length are positive non-zero
integers before using them: after converting argv to int with std::atoi for
variables runs and chain_length, check if the result is <= 0 (or if conversion
failed) and either set a safe default or print an error and exit; for
chain_length also ensure it is at least 1 and then clamp it to
ALL_HEADERS.size() to avoid unsigned underflow when using chain_length in later
logic (references: variables runs, chain_length, and ALL_HEADERS).

---

520-528: ⚠️ Potential issue | 🟡 Minor

Guard against division by zero in speedup calculation.

If chain_med is 0 (extremely fast timing or measurement artifact), mono_med / chain_med causes undefined behavior or infinity.

🛡️ Suggested fix

         std::println("  Monolithic full rebuild:  median {:.1f}ms", mono_med);
         std::println("  Chained append-one-link:  median {:.1f}ms", chain_med);
-        std::println("  Speedup: {:.1f}x", mono_med / chain_med);
+        if(chain_med > 0)
+            std::println("  Speedup: {:.1f}x", mono_med / chain_med);
+        else
+            std::println("  Speedup: N/A (chain time is zero)");
     }

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 520 - 528, The
speedup calculation can divide by zero when chain_med == 0; update the block
that computes mono_med and chain_med (variables mono_times, chain_times,
mono_med, chain_med and the std::println that prints "Speedup") to guard against
zero or near-zero chain_med (e.g., check chain_med != 0 or chain_med > a small
epsilon) and print a safe placeholder like "inf" or "N/A" (or skip the speedup
line) when chain_med is zero instead of performing mono_med / chain_med.

---

923-927: ⚠️ Potential issue | 🟡 Minor

Guard against division by zero in speedup calculation.

Same pattern as other benchmarks - if chain_med is 0, the division causes undefined behavior.

🛡️ Suggested fix

             if(!mono_rebuild_times.empty()) {
                 double mono_med = mono_rebuild_times[mono_rebuild_times.size() / 2];
-                std::println("    Speedup: {:.0f}x", mono_med / chain_med);
+                if(chain_med > 0)
+                    std::println("    Speedup: {:.0f}x", mono_med / chain_med);
+                else
+                    std::println("    Speedup: N/A");
             }

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 923 - 927, The
speedup print block can divide by zero when chain_med == 0; in the section that
computes mono_med and prints Speedup (the block using mono_rebuild_times,
mono_med and chain_med) add a guard: only compute and print mono_med / chain_med
when chain_med is non‑zero (or above a tiny epsilon), otherwise handle the zero
case (skip printing, print "N/A"/"inf"/a warning message) to avoid undefined
behavior.

---

145-153: ⚠️ Potential issue | 🔴 Critical

Critical: Static storage causes dangling pointers when function is called multiple times.

The static std::vector<std::string> storage is overwritten on each call, but the returned const char* pointers from previous calls still reference the now-invalidated string data. This causes undefined behavior when build_one_pch is called in loops (e.g., in build_chain), as cp.arguments from earlier iterations becomes dangling before compile() completes.

🐛 Proposed fix: Return owned storage alongside args

-/// Build compiler arguments for PCH generation.
-static std::vector<const char*> make_pch_args(const std::string& file,
-                                              const std::string& resource_dir) {
-    static std::vector<std::string> storage;
-    storage = {"clang++", "-std=c++20", "-resource-dir", resource_dir, "-x", "c++-header", file};
-    std::vector<const char*> args;
-    for(auto& s: storage)
-        args.push_back(s.c_str());
-    return args;
-}
+/// Build compiler arguments for PCH generation.
+/// Returns {owned_strings, arg_ptrs} - caller must keep owned_strings alive.
+static std::pair<std::vector<std::string>, std::vector<const char*>>
+make_pch_args(const std::string& file, const std::string& resource_dir) {
+    std::vector<std::string> storage =
+        {"clang++", "-std=c++20", "-resource-dir", resource_dir, "-x", "c++-header", file};
+    std::vector<const char*> args;
+    for(const auto& s: storage)
+        args.push_back(s.c_str());
+    return {std::move(storage), std::move(args)};
+}

Then update the call site in build_one_pch (line 181-182):

-    auto args = make_pch_args(file_path, resource_dir);
-    cp.arguments = args;
+    auto [arg_storage, args] = make_pch_args(file_path, resource_dir);
+    cp.arguments = args;
+    // arg_storage must remain alive until compile() returns

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 145 - 153, The
function make_pch_args uses a static std::vector<std::string> storage which gets
overwritten across calls causing previously returned const char* pointers to
dangle; change make_pch_args to return owned storage alongside the C-string
pointers (e.g., return a std::pair<std::vector<std::string>, std::vector<const
char*>> or accept an output std::vector<std::string>& storage param) so the
string storage remains alive for the lifetime of cp.arguments, and update the
caller build_one_pch to hold that returned storage (or pass in a storage
variable) and set cp.arguments from the associated vector<const char*> before
calling compile() to avoid dangling pointers for cp.arguments used in
build_chain.

benchmarks/pch_chain/pch_chain_benchmark.cpp (2)

1006-1033: Resource directory detection assumes specific build layout.

The code assumes the executable is in <build>/bin/ with resources at <build>/lib/clang/. This may not match all build configurations (e.g., installed locations, out-of-tree builds). Consider adding an environment variable fallback or --resource-dir CLI option.

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 1006 - 1033, The
resource_dir discovery logic (the block that sets resource_dir using argv[0],
llvm::sys::fs::exists and the directory_iterator) assumes a specific build
layout; update the program to accept an explicit resource directory via a new
CLI flag (--resource-dir) and/or an environment variable fallback (e.g.,
RESOURCE_DIR) before falling back to the current argv[0]-based lookup, and use
that value to set resource_dir; modify the argument parsing (or main) to check
the new flag/env var first and only run the existing llvm::sys::path discovery
if no explicit path is provided.

---

1014-1014: Hardcoded LLVM version "21" may break with future versions.

The path lib/clang/21 assumes LLVM 21.x. While there's a fallback scan, consider extracting this as a constant or using only the version-agnostic scan.

♻️ Suggested improvement

+    // LLVM major version - update when upgrading LLVM
+    constexpr const char* LLVM_MAJOR_VERSION = "21";
+
     // Find resource dir from argv[0].
     std::string resource_dir;
     {
         // ...
-        llvm::sys::path::append(rd, "lib", "clang", "21");
+        llvm::sys::path::append(rd, "lib", "clang", LLVM_MAJOR_VERSION);

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` at line 1014, Replace the
hardcoded "21" in the llvm::sys::path::append call with a version-agnostic
approach: either extract the version string into a constant (e.g., const char*
CLANG_DEFAULT_VERSION = "21") and use that in llvm::sys::path::append(rd, "lib",
"clang", CLANG_DEFAULT_VERSION), or better, perform a directory scan of
rd/"lib"/"clang" to select the available/highest clang subdirectory and append
that name instead; update places that reference llvm::sys::path::append(rd,
"lib", "clang", "21") (look for the exact call) to use the new constant or the
scanned directory name so future LLVM versions won’t break.

Fix all unresolved CodeRabbit comments on this PR:

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

benchmarks/pch_chain/pch_chain_benchmark.cpp (3)

520-527: ⚠️ Potential issue | 🟡 Minor

Guard benchmark ratio denominators before printing speedups.

chain_med/mono_med can be 0.0 for very short measurements, which prints inf/nan and makes the benchmark output misleading. Add denominator checks before each ratio.

🛡️ Proposed pattern

-            std::println("  Speedup: {:.1f}x", mono_med / chain_med);
+            if(chain_med > 0.0)
+                std::println("  Speedup: {:.1f}x", mono_med / chain_med);
+            else
+                std::println("  Speedup: N/A (chained median is zero)");

-            double ratio = chain_med / mono_med;
-            std::println("  Ratio (chained/mono): {:.2f}x", ratio);
+            if(mono_med > 0.0)
+                std::println("  Ratio (chained/mono): {:.2f}x", chain_med / mono_med);
+            else
+                std::println("  Ratio (chained/mono): N/A (monolithic median is zero)");

Also applies to: 789-793, 921-928

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 520 - 527, The
printing of speedup ratios uses mono_med and chain_med as denominators and can
produce inf/nan when either is 0.0; update the blocks that compute and print
ratios (the variables mono_med and chain_med and the std::println calls that
print "Speedup") to check denominators before dividing and only print the ratio
when the denominator is nonzero (otherwise print a clear placeholder like "N/A"
or "—"); apply the same guard to the other similar sections referenced (around
lines with mono_med/chain_med at 789-793 and 921-928).

---

37-53: ⚠️ Potential issue | 🟡 Minor

Keep the benchmark header set aligned with -std=c++20.

ALL_HEADERS still includes <expected>, but every benchmark compile invocation uses -std=c++20. That makes the “C++20 standard library headers” set inaccurate and may fail or silently exclude the API depending on the standard library implementation. Remove "expected" or switch the child compile flags to C++23 consistently.

🛠️ Proposed fix if this benchmark should remain C++20-only

     "variant",
     "any",
-    "expected",
     "bitset",

Also applies to: 145-148, 242-243, 550-551

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 37 - 53,
ALL_HEADERS includes "expected" which is a C++23 header while the benchmarks
compile with -std=c++20; either remove the "expected" entry from the ALL_HEADERS
vector (and any other header lists that include "expected") so the header set is
C++20-safe, or alternatively update the benchmark compile flags to use
-std=c++23 consistently; locate the ALL_HEADERS vector definition and other
occurrences that list headers (the string "expected") and either delete that
entry or change the build invocation to C++23 across the benchmark compile
steps.

---

997-1002: ⚠️ Potential issue | 🟡 Minor

Validate CLI values before using them in benchmark bounds.

std::atoi() turns non-numeric input into 0, and --chain-length 0 still reaches chain_length - 1, causing unsigned underflow in the incremental benchmark call.

🛡️ Proposed fix

         std::string arg = argv[i];
         if(arg == "--runs" && i + 1 < argc) {
             runs = std::atoi(argv[++i]);
+            if(runs <= 0) {
+                std::println(stderr, "Runs must be positive");
+                return 1;
+            }
         } else if(arg == "--chain-length" && i + 1 < argc) {
-            chain_length = std::atoi(argv[++i]);
+            int parsed = std::atoi(argv[++i]);
+            if(parsed <= 0) {
+                std::println(stderr, "Chain length must be at least 1");
+                return 1;
+            }
+            chain_length = static_cast<std::size_t>(parsed);
             if(chain_length > ALL_HEADERS.size())
                 chain_length = ALL_HEADERS.size();

Also applies to: 1049-1051

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 997 - 1002, The
CLI parsing uses std::atoi (variables runs and chain_length) which yields 0 for
non-numeric input and allows chain_length==0 leading to unsigned underflow;
change parsing to validate inputs (e.g., use std::strtol or std::stoi with error
checking or check that the argv token consists only of digits) and then
clamp/validate values before use: ensure runs > 0 and 1 <= chain_length <=
ALL_HEADERS.size(), or set a safe default/fail with a clear message if invalid.
Apply the same validation/clamping logic to the other parsing site that sets
chain_length (the second occurrence of the argv parsing block) so both places
prevent zero or out-of-range values.

benchmarks/pch_chain/pch_chain_benchmark.cpp (1)

1009-1039: Prefer the existing resource-dir helper over custom path probing.

This duplicates resource-dir discovery and hardcodes the lib/clang/21 layout. The shared helper in src/command/argument_parser.cpp already delegates to Clang’s resource-path discovery and is more robust for installed/symlinked binaries.

🤖 Prompt for AI Agents

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

In `@benchmarks/pch_chain/pch_chain_benchmark.cpp` around lines 1009 - 1039,
Replace the custom resource-dir probing block that populates the variable
resource_dir (the whole scope that builds bin_path/bin_dir/build_dir and
searches lib/clang/21) with a call to the shared resource-dir helper provided in
src/command/argument_parser.cpp; remove the hardcoded "lib/clang/21" logic and
instead call the argument_parser helper to obtain the clang resource directory
and assign its result to resource_dir, preserving the existing error path that
checks resource_dir.empty() and the subsequent error message.

Fix all unresolved CodeRabbit comments on this PR:

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