Skip to content

fix(ini): retain nested parse error details - #323

Merged
fbraz3 merged 2 commits into
mainfrom
fix/issue-297-ini-diagnostics
Sep 21, 2026
Merged

fbraz3 merged 2 commits into
mainfrom
fix/issue-297-ini-diagnostics

Conversation

@arazmj

@arazmj arazmj commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

A bad value inside a nested INI module should be reported at that field, not
replaced by the outer Object ... header. That loss of context has made #297
much harder to investigate.

This keeps an existing INIException when it passes through the module and
top-level block handlers. Numeric std::from_chars failures also print the
token and conversion reason to stderr, so release builds retain that clue.
File cleanup and the original failure path remain intact.

There is no clamping, ignored-field behavior, parsing fallback, or change to
accepted values. The shared implementation covers both game variants.

The supplied Shockwave files contain oversized SpawnReplaceDelay and
RecenterTime values that exceed the signed 64-bit intermediate used by the
Linux parser. Those are confirmed rejection points, but we have not established
which field fails first in the full mod. The originally suspected W3D fields
are already implemented.

Related to #297. This improves diagnostics; it does not fix or close the
Shockwave compatibility issue.
The separate vanilla-startup fix in #308 is
still relevant for the reporter's current Flatpak setup. The pending #321
parseBitString8 and recoil changes address different paths.

Validation

  • An isolated harness using parser-function extracts and each game's actual
    exception header reproduced the baseline's lost context, then exercised the
    corrected nested exception propagation with ASan and UBSan.
  • Exercised both from_chars and sscanf paths, including valid numbers, +
    prefixes, the -1 sentinel, numeric suffix behavior and cleanup.
  • Syntax-checked the actual patched INI.cpp using the generated macOS compiler
    configurations for Generals and Zero Hour.
  • git diff --check passes.

No full Shockwave run, retail overflow-behavior comparison, or installed-game
changes are claimed.

Summary by CodeRabbit

  • Bug Fixes

    • Improved configuration parsing errors by preserving the original error details instead of replacing them with less-specific messages.
    • Added clearer diagnostics for invalid or out-of-range numeric values, helping identify the type of parsing failure and affected input.
    • Existing numeric conversion behavior remains unchanged.
  • Documentation

    • Updated the development worklog with details about parsing diagnostics, validation, identified input issues, and testing.

Preserve existing INI exceptions through outer parsing handlers and log
numeric conversion failures without changing accepted values or recovery.

Related to #297

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@arazmj arazmj added the bug Something isn't working label Sep 21, 2026
@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: fbraz3/GeneralsX/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 5136a59e-d61c-4626-9982-facf0c67cccb

📥 Commits

Reviewing files that changed from the base of the PR and between 9d2cb11 and 3c6d0f0.

📒 Files selected for processing (1)
  • docs/WORKLOG/2026-09-DIARY.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/WORKLOG/2026-09-DIARY.md

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

INI loading now preserves nested INIException details. Numeric conversion failures now log the token and classify invalid or out-of-range results before throwing INI_INVALID_DATA. The worklog records these changes and related validation.

Changes

INI diagnostics

Layer / File(s) Summary
Nested exception preservation
Core/GameEngine/Source/Common/INI/INI.cpp
INI::load and INI::initFromINIMulti rethrow existing INIException instances without replacing their diagnostics.
Numeric conversion diagnostics
Core/GameEngine/Source/Common/INI/INI.cpp, docs/WORKLOG/2026-09-DIARY.md
Floating-point and integral parsing now logs the token and identifies invalid or out-of-range failures. The worklog records the changes and validation details.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: fbraz3

🚥 Pre-merge checks | ✅ 10
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits format, uses the valid type fix, contains an appropriate ini scope, and accurately describes preservation of nested parse error details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Platform Isolation ✅ Passed PASS. The PR changes only Core/GameEngine/Source/Common/INI/INI.cpp and a worklog. The added engine code uses C++ exception handling, std::from_chars/std::errc, and standard C fprintf, `fflush…
Cross-Platform Determinism ✅ Passed PASS: The pull request changes only Core/GameEngine/Source/Common/INI/INI.cpp and a worklog. The source changes preserve INIException objects and add std::from_chars error logging. They do not a…
Openal / Miniaudio Parity ✅ Passed PASS. The reviewed range changes only Core/GameEngine/Source/Common/INI/INI.cpp and the worklog. The patch contains no OpenAL or MiniAudio changes, and all identified OpenAL/MiniAudio implementation…
Conventional Commit Standards ✅ Passed All non-merge commit subjects in the reviewed range follow Conventional Commits: fix(ini): retain nested parse error details. The other commit has two parents and is a true merge commit, so the chec…
No Hardcoded Local Paths / Sensitive Info ✅ Passed PASS: The pull request adds only INI exception/diagnostic code and a worklog entry. The added lines contain no personal machine paths, private environment variables, internal URLs, or local workspace …
Ngmp Protocol Integrity ✅ Passed PASS — The authoritative pull-request diff changes only Core/GameEngine/Source/Common/INI/INI.cpp and the worklog. It contains no changes under NGMP, cmake/ngmp.cmake, NGMP UI, loading-screen tick…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

Nested errors keep their name,
Numeric logs explain the game,
Invalid or range-bound clues,
Stay intact through parsing queues,
Cross-platform checks mark the frame.

Comment @coderabbitai help to get the list of available commands.

@fbraz3

fbraz3 commented Sep 21, 2026

Copy link
Copy Markdown
Owner

@coderabbitai please resolve conflicts

@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

🤖 Completed: Resolve merge conflicts in PR #323 — View commit 3c6d0f0

Resolved conflicts in:
- docs/WORKLOG/2026-09-DIARY.md (unmerged)

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
CodeRabbit-Task-Id: 84f7a3b0-781b-4b0e-a248-2736b719b63b
@fbraz3
fbraz3 merged commit 5b204e1 into main Sep 21, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants