Skip to content

fix(cli): validate numeric arguments without truncation - #346

Open
blackdragoon26 wants to merge 4 commits into
1a1a11a:developfrom
blackdragoon26:fix/cli-numeric-parsing
Open

blackdragoon26 wants to merge 4 commits into
1a1a11a:developfrom
blackdragoon26:fix/cli-numeric-parsing

Conversation

@blackdragoon26

@blackdragoon26 blackdragoon26 commented Oct 12, 2026 •

Copy link
Copy Markdown

What does this PR do?

Fixes numeric option parsing in cachesim. Malformed values such as --hashpower=8junk and --sample-ratio=nan now produce an error instead of silently changing simulation settings.

Request limits are parsed as 64-bit integers, so --num-req=4294967306 no longer becomes 10. Existing supported values, including --num-req=-1, are preserved.

Related to #206.

Type of change

  • Bug fix
  • New eviction / admission / prefetch algorithm
  • New trace reader or trace format
  • Performance improvement
  • Documentation
  • Build / CI
  • Other:

How was it tested?

Tested on macOS with a Debug build and Ubuntu x86_64 with a Release build and LeakSanitizer. All 10 CTest targets pass. The latest macOS run includes 262 CLI checks, all passing.

Formatting, shell syntax and whitespace checks pass. The existing macOS warning in libCacheSim/bin/debug/main.cpp remains.

  • ctest --test-dir _build --output-on-failure passes, tested using _build_dbg
  • Build is warning-free (CI uses -Wall -Wextra -Werror)

Results

Invalid numeric arguments are rejected with a clear error. Large request limits retain their full value, and valid inputs continue to work.

Checklist

  • Formatted with clang-format (or the pre-commit hook from scripts/setup_hooks.sh)
  • Added or updated tests
  • Added or updated documentation
  • New algorithms are registered in the CLI and listed in the README

Signed-off-by: blackdragoon26 <sankalp.jha9643@gmail.com>
@1a1a11a
1a1a11a requested a balanced review from Copilot October 17, 2026 13:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@blackdragoon26
blackdragoon26 requested a balanced review from Copilot October 17, 2026 13:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@haochengxia

Copy link
Copy Markdown
Collaborator

Thanks! LGTM, and should we reject extreme small sample ratio?
Consider one case:

s->sampling_ratio = sampling_ratio;
s->sampling_ratio_inv = (int)(1.0 / sampling_ratio); -> would overflow with 1e-999

Signed-off-by: blackdragoon26 <sankalp.jha9643@gmail.com>
@blackdragoon26

Copy link
Copy Markdown
Author

Thanks! LGTM, and should we reject extreme small sample ratio?
Consider one case:

s->sampling_ratio = sampling_ratio;
s->sampling_ratio_inv = (int)(1.0 / sampling_ratio); -> would overflow with 1e-999

Thank you for catching this! Added the lower-bound check and boundary tests in ecbcfcf.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants