fix: case-insensitive arguments. - #42
Conversation
- Normalize arguments to lowercase before comparison so that --codec-CPU etc. will still be accepted as valid flags instead of being silenlty ignored.
📝 WalkthroughWalkthroughCommand-line parsing in ChangesCLI option parsing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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 `@src/main.cpp`:
- Around line 158-164: Restrict the lowercase transformation in the
argument-normalization block to dash-prefixed flags longer than two characters,
preserving case for short options such as -M, -H, and -P. Keep the existing
long-option matching flow unchanged and ensure comparisons against uppercase
short flags remain reachable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
|
||
| // Normalize flag arguments to lowercase for case-insensitive matching | ||
| std::string arg_lower = arg; | ||
| if (!arg.empty() && arg[0] == '-') { | ||
| std::transform(arg_lower.begin(), arg_lower.end(), arg_lower.begin(), | ||
| [](unsigned char c) { return static_cast<char>(std::tolower(c)); }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Case-insensitive short options create collisions and dead code.
Lowercasing all dash-prefixed arguments introduces critical logic errors for short options that rely on case sensitivity:
-M(Metal) is lowercased to-m, colliding with--modelat line 166.-H(Host) is lowercased to-h, colliding with--helpat line 234.-P(Port) is lowercased to-p, which skips the condition chain entirely.
Additionally, comparisons like arg_lower == "-M" and arg_lower == "-H" become dead code because arg_lower will never contain uppercase letters after the transformation.
To fix this while keeping long options case-insensitive, restrict the lowercase transformation to flags longer than 2 characters. This correctly preserves standard short flags like -M, -H, and -P.
🐛 Proposed fix
- if (!arg.empty() && arg[0] == '-') {
+ if (arg.length() > 2 && arg[0] == '-') {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Normalize flag arguments to lowercase for case-insensitive matching | |
| std::string arg_lower = arg; | |
| if (!arg.empty() && arg[0] == '-') { | |
| std::transform(arg_lower.begin(), arg_lower.end(), arg_lower.begin(), | |
| [](unsigned char c) { return static_cast<char>(std::tolower(c)); }); | |
| } | |
| // Normalize flag arguments to lowercase for case-insensitive matching | |
| std::string arg_lower = arg; | |
| if (arg.length() > 2 && arg[0] == '-') { | |
| std::transform(arg_lower.begin(), arg_lower.end(), arg_lower.begin(), | |
| [](unsigned char c) { return static_cast<char>(std::tolower(c)); }); | |
| } |
🤖 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 `@src/main.cpp` around lines 158 - 164, Restrict the lowercase transformation
in the argument-normalization block to dash-prefixed flags longer than two
characters, preserving case for short options such as -M, -H, and -P. Keep the
existing long-option matching flow unchanged and ensure comparisons against
uppercase short flags remain reachable.
…n normalizing CLI arguments.
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 `@src/main.cpp`:
- Line 76: Fix the Metal CLI contract around the help text in main.cpp and the
argument normalization/dispatch logic: either update README.md to document the
actual -m form, or preserve the documented -M alias by ensuring normalization
does not convert it into the model option and dispatches it as Metal. Apply the
same correction to both Metal help entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| safe_print(" -v, --vulkan <id> Vulkan device index (-1 = CPU)\n"); | ||
| safe_print(" -c, --cuda <id> CUDA device index (-1 = CPU)\n"); | ||
| safe_print(" -M, --metal Use Metal backend (macOS only)\n"); | ||
| safe_print(" -mt, --metal Use Metal backend (macOS only)\n"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the documented Metal flag or preserve -M as an alias.
README.md still documents -M, --metal, but normalization converts -M to -m, which dispatches it as the model option instead of selecting Metal. This breaks the existing documented CLI contract.
Proposed documentation fix
- | `-M`, `--metal` | off | Use the default Metal device on macOS |
+ | `-mt`, `--metal` | off | Use the default Metal device on macOS |Also applies to: 178-178
🤖 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 `@src/main.cpp` at line 76, Fix the Metal CLI contract around the help text in
main.cpp and the argument normalization/dispatch logic: either update README.md
to document the actual -m form, or preserve the documented -M alias by ensuring
normalization does not convert it into the model option and dispatches it as
Metal. Apply the same correction to both Metal help entries.
1 participant
Summary by CodeRabbit
--Log-Levelare accepted consistently.-Mto-mtto match the supported flag.