Debugger: Allow specification of several breakpoints from cmdline args #1075
Conversation
WalkthroughThe pull request modifies the breakpoint handling in the debugger's command-line interface. The key change is transforming the breakpoint storage from a single optional string to a vector of strings, enabling multiple breakpoints to be set simultaneously. This modification affects the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
debugger/src/main.rs (2)
329-331: Remove unnecessary space after&operator.The iteration logic correctly handles multiple breakpoints, but there's a minor style issue.
- context.breakpoint(& breakpoint); + context.breakpoint(&breakpoint);
Line range hint
290-301: Update help text to document multiple breakpoint support.The help text should mention that multiple breakpoints can be specified using the
-bor--breakpointoption multiple times.Options:\n\ -g, --grammar <grammar file> - load .pest grammar\n\ -i, --input <input file> - load input file\n\ -r, --rule <rule> - run rule\n\ - -b, --breakpoint <rule> - breakpoint at rule\n\ + -b, --breakpoint <rule> - breakpoint at rule (can be specified multiple times)\n\ -s, --session <session file> - load session history file\n\ -h, --help - print this help menu\n\
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
debugger/src/main.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Unit, Style, and Lint Testing
- GitHub Check: check for semver compatibility
- GitHub Check: check for no_std compatibility
- GitHub Check: cargo hack check --feature-powerset
- GitHub Check: Test Coverage
- GitHub Check: Documentation check
- GitHub Check: Minimal Versions Testing
🔇 Additional comments (2)
debugger/src/main.rs (2)
225-225: LGTM! Field change enables multiple breakpoints.The change from
Option<String>toVec<String>appropriately supports storing multiple breakpoints, aligning with the PR objectives.
236-236: LGTM! Breakpoint collection logic is correct.The implementation correctly:
- Initializes an empty vector for breakpoints
- Accumulates multiple breakpoints via
push- Preserves error handling for missing arguments
Also applies to: 271-275
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
debugger/src/main.rs (1)
Line range hint
291-301: Update help text to reflect multiple breakpoint support.The help text still indicates single breakpoint usage. Consider updating it to clarify that multiple breakpoints can be specified.
- -b, --breakpoint <rule> - breakpoint at rule\n\ + -b, --breakpoint <rule> - add breakpoint at rule (can be specified multiple times)\n\
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
debugger/src/main.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: check for semver compatibility
- GitHub Check: check for no_std compatibility
- GitHub Check: cargo hack check --feature-powerset
- GitHub Check: Unit, Style, and Lint Testing
- GitHub Check: Minimal Versions Testing
- GitHub Check: Test Coverage
- GitHub Check: Documentation check
🔇 Additional comments (4)
debugger/src/main.rs (4)
225-225: LGTM! Field type change enables multiple breakpoints.The change from
Option<String>toVec<String>appropriately supports storing multiple breakpoints.
236-236: LGTM! Default implementation correctly handles multiple breakpoints.The implementation properly initializes an empty vector and accumulates breakpoints through the push operation while preserving error handling.
Also applies to: 271-275
329-331: LGTM! Init method properly sets multiple breakpoints.The implementation correctly iterates over the breakpoints vector to set each breakpoint individually.
Line range hint
225-331: Consider adding tests for multiple breakpoint functionality.The new multiple breakpoint feature would benefit from test coverage to ensure reliability. Consider adding tests for:
- Setting multiple breakpoints via command line
- Verifying all breakpoints are active
- Edge cases like duplicate breakpoints
Would you like me to help generate test cases for these scenarios?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
debugger/src/main.rs (1)
329-331: Consider adding error handling for individual breakpoints.While the implementation correctly processes multiple breakpoints, consider adding error handling to report issues with individual breakpoints without failing the entire initialization.
for breakpoint in self.breakpoints { - context.breakpoint(&breakpoint); + if !context.context.rule_exists(&breakpoint) { + eprintln!("Warning: Rule '{}' not found in grammar", breakpoint); + } + context.breakpoint(&breakpoint); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
debugger/src/main.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Unit, Style, and Lint Testing
- GitHub Check: check for semver compatibility
- GitHub Check: check for no_std compatibility
- GitHub Check: Test Coverage
- GitHub Check: cargo hack check --feature-powerset
- GitHub Check: Documentation check
- GitHub Check: Minimal Versions Testing
🔇 Additional comments (3)
debugger/src/main.rs (3)
225-225: LGTM! Field type change enables multiple breakpoints.The change from
Option<String>toVec<String>appropriately supports storing multiple breakpoints.
236-236: LGTM! Default implementation correctly handles multiple breakpoints.The implementation properly initializes an empty vector and correctly accumulates breakpoints from command line arguments while preserving error handling.
Also applies to: 271-275
297-297: LGTM! Help message clearly indicates multiple breakpoints support.The help message update appropriately informs users about the ability to specify multiple breakpoints.
That nitpick isn't straightforward to address, as there's easy way to list rules in the DebuggerContext ( |
done. |
Head branch was pushed to by a user without write access
zaddach
force-pushed
the
multiple_breakpoints_from_cmdline
branch
from
85dfb9c to
6d4bbcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
debugger/src/main.rs (1)
329-331: Consider implementing error handling for individual breakpoints.While the iteration logic is correct, consider implementing error handling for individual breakpoints as suggested in the PR comments. This would allow reporting issues with specific breakpoints without failing the entire initialization process.
Consider this approach:
- for breakpoint in self.breakpoints { - context.breakpoint(&breakpoint); - } + for breakpoint in self.breakpoints { + if let Err(e) = context.context.check_rule_exists(&breakpoint) { + eprintln!("Warning: Invalid breakpoint '{}': {}", breakpoint, e); + } else { + context.breakpoint(&breakpoint); + } + }Note: This suggestion assumes the addition of a
check_rule_existsmethod toDebuggerContextas mentioned in the PR comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
debugger/src/main.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: check for semver compatibility
- GitHub Check: check for no_std compatibility
- GitHub Check: Unit, Style, and Lint Testing
- GitHub Check: cargo hack check --feature-powerset
- GitHub Check: Minimal Versions Testing
- GitHub Check: Test Coverage
- GitHub Check: Documentation check
- GitHub Check: Fuzzing
🔇 Additional comments (3)
debugger/src/main.rs (3)
225-225: LGTM! Field type change enables multiple breakpoints.The change from
Option<String>toVec<String>correctly implements the ability to store multiple breakpoints.
236-236: LGTM! Default implementation correctly handles multiple breakpoints.The initialization with an empty vector and the push operation correctly implement the accumulation of multiple breakpoints while preserving error handling.
Also applies to: 271-271
297-297: LGTM! Help message clearly indicates multiple breakpoints support.The updated help message accurately reflects the new functionality and maintains consistency with other help entries.
Currently, only a single
-b|--breakpointargument is accepted from command line. I've changed the argument parsing so that the argument can be specified several times to add several different breakpoints.Summary by CodeRabbit