-
-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable logging for responses with only alloc. #78
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, just as I wrap up opening #79, I realize you've opened a PR fixing it. Thank you!
As per my suggestion in #79, instead of overloading the "alloc"
feature here, can you add a new feature to Cargo.toml
called "trace-pkt"
? This feature would have a dependency on alloc
, while not forcing this additional logging overhead on all alloc
users.
You'd also want to cfg-gate the incoming packet trace statement, which is currently always enabled.
Ah, also, I'd make |
Also feature gate imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd also want to cfg-gate the incoming packet trace statement, which is currently always enabled.
i.e: please also feature = "trace-pkt"
gate the following lines:
https://github.com/daniel5151/gdbstub/blob/dev/0.6/src/protocol/recv_packet.rs#L56-L59
https://github.com/daniel5151/gdbstub/blob/dev/0.6/src/protocol/recv_packet.rs#L114-L117
EDIT: aaaand you beat me to the punch again 😅
I think I did this in 9e060d2 but let me know if I missed something. |
Awesome, thanks again! |
Description
Enable logging for responses (as discussed here #77)
API Stability
Checklist
cargo build
compiles withouterrors
orwarnings
cargo fmt
was run