fix(Pythia6Generator): replace fscanf parsing with istringstream#1260
fix(Pythia6Generator): replace fscanf parsing with istringstream#1260olantwin wants to merge 1 commit into
Conversation
clang-tidy's bugprone-unchecked-string-to-number-conversion flagged both fscanf calls in Pythia6Generator::ReadEvent — the format-string based parser silently produces zeros when an input field doesn't match, instead of surfacing the parse error to the caller. Rewrite with fgets + std::istringstream. Each line is read into a buffer, then >> extracts the typed fields one at a time. If any extraction fails, log the offending line and abort the event, the same way std::stoi/std::stod handle malformed input. The header line and the per-track line both get explicit fail checks. No functional change for well-formed input files; malformed input that previously fell through with zero fields now fails closer to the cause.
📝 WalkthroughWalkthrough
ChangesPythia6Generator ReadEvent Parsing Rewrite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
CI Plots
|
|
@coderabbitai rate limit |
|
Your plan includes 1 PR review per hour. More reviews will be available in 16 minutes and 41 seconds. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shipgen/Pythia6Generator.cxx`:
- Around line 72-76: The error message in the header parsing block incorrectly
reports "End of input file reached" for both malformed input (extraction
failures) and invalid semantics (ntracks <= 0). Separate the combined condition
check into two distinct checks: first test whether the extraction of eventID and
ntracks succeeded, and if that fails, log an error message indicating malformed
header with the raw line content; separately check if ntracks <= 0 and log a
different error message for invalid track count. This approach aligns with how
track parsing errors are handled at lines 93-94 (which log "malformed track
line" with raw content), ensuring consistent error reporting and improving
debuggability.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5087b95d-ce4c-49b7-8ea6-928566e1ea3d
📒 Files selected for processing (1)
shipgen/Pythia6Generator.cxx
| if (!(headerStream >> eventID >> ntracks) || ntracks <= 0) { | ||
| cout << "-I Pythia6Generator: End of input file reached " << endl; | ||
| CloseInput(); | ||
| return kFALSE; | ||
| } |
There was a problem hiding this comment.
Misleading error message for malformed event headers.
Lines 66–67 already handle EOF when fgets returns nullptr. If we reach line 72, we have a buffer containing a line. The combined condition !(headerStream >> eventID >> ntracks) || ntracks <= 0 fails when:
- Extraction fails (malformed input, e.g., non-numeric tokens), or
- Extraction succeeds but
ntracks ≤ 0(semantically invalid).
In both cases, the message "End of input file reached" is incorrect—the actual problem is malformed or invalid input, not EOF. For comparison, track parsing errors (lines 93–94) log "malformed track line" and include the raw line content. The header error should follow the same pattern for consistency and to aid debugging.
📝 Proposed fix: separate parse/validation checks and log the malformed line
std::istringstream headerStream(headerBuf);
- if (!(headerStream >> eventID >> ntracks) || ntracks <= 0) {
- cout << "-I Pythia6Generator: End of input file reached " << endl;
+ if (!(headerStream >> eventID >> ntracks)) {
+ LOG(error) << "Pythia6Generator: malformed event header: " << headerBuf;
+ CloseInput();
+ return kFALSE;
+ }
+ if (ntracks <= 0) {
+ LOG(error) << "Pythia6Generator: invalid track count " << ntracks
+ << " in event " << eventID;
CloseInput();
return kFALSE;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shipgen/Pythia6Generator.cxx` around lines 72 - 76, The error message in the
header parsing block incorrectly reports "End of input file reached" for both
malformed input (extraction failures) and invalid semantics (ntracks <= 0).
Separate the combined condition check into two distinct checks: first test
whether the extraction of eventID and ntracks succeeded, and if that fails, log
an error message indicating malformed header with the raw line content;
separately check if ntracks <= 0 and log a different error message for invalid
track count. This approach aligns with how track parsing errors are handled at
lines 93-94 (which log "malformed track line" with raw content), ensuring
consistent error reporting and improving debuggability.
1 participant
Summary
clang-tidy's
bugprone-unchecked-string-to-number-conversionflagged bothfscanfcalls inPythia6Generator::ReadEvent(the remaining 2 sites from that category after PR #1244 swept atoi/atof).fscanfsilently produces zeros when an input field doesn't match the format — the value just stays at its prior contents — instead of surfacing the parse error to the caller.Change
Rewrite with
fgets+std::istringstream. Each line is read into a buffer, then>>extracts the typed fields one at a time. If any extraction fails, log the offending line and abort the event, matching thestd::stoi/std::stoderror model PR #1244 adopted.Both the header line (
eventID,ntracks) and the per-track line (15 fields) get explicit fail checks. End-of-file detection now naturally drops out offgetsreturningnullptr.Test plan
pixi run --locked build— clean (133/133, 0 warnings)pixi run --locked test— 2/2 passCPP_FILES=shipgen/Pythia6Generator.cxx pixi run --locked ci-clang-tidy— 0bugprone-unchecked-string-to-number-conversionfindingsci-fixed-targetjob still passes (its Pythia6 input is well-formed, so behaviour is unchanged)Risks
Malformed Pythia6 input lines that previously fell through with zero fields will now log an error and skip the event. If any existing input file relies on the zero-fallback, this will surface as a runtime failure with a clear message — preferred to silent corruption.
Summary by CodeRabbit