-
Notifications
You must be signed in to change notification settings - Fork 56
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
#534 Support for line debugging #656
#534 Support for line debugging #656
Conversation
…etting-breakpoints-on-different-lines-in-code
…etting-breakpoints-on-different-lines-in-code
0527b99
to
37bf7b8
Compare
Local variables and parameters are now supported
…etting-breakpoints-on-different-lines-in-code
dd5d20c
to
8d3eeb8
Compare
Codecov ReportBase: 94.09% // Head: 94.11% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #656 +/- ##
==========================================
+ Coverage 94.09% 94.11% +0.01%
==========================================
Files 46 46
Lines 18245 18763 +518
==========================================
+ Hits 17168 17658 +490
- Misses 1077 1105 +28
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…ifferent-lines-in-code
…etting-breakpoints-on-different-lines-in-code
…ifferent-lines-in-code
…etting-breakpoints-on-different-lines-in-code
src/codegen/debug.rs
Outdated
scope: DIScope<'ink>, | ||
line: usize, | ||
column: usize, | ||
) -> Result<DILocation, Diagnostic> { |
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.
Result not required - in fact I believe most of these functions will never return an Err?
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.
They should, I think I mostly forgot to remove / change some panics, I'll take another look
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.
Correction: I had not seen the methods this is on, this one was useless i removed the method
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.
3 methods in DebugBuilder impl look like they will never fail
others could benefit from a .expect(...)( when calling types.get(...)
src/codegen/debug.rs
Outdated
.expect("A POU will have an impl at this stage"); | ||
if implementation.implementation_type != ImplementationType::Function { | ||
if implementation.get_implementation_type() == &ImplementationType::Method { | ||
todo!("Method debugging"); |
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.
will codegen now panic for methods?
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.
not unless it's being compiled with -g, but I could make this a diagnostic instead.
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.
its now an Err(...). we should just ignore it for the moment so a method will not permit a debug-build
src/codegen/debug.rs
Outdated
.map(|dt| { | ||
self.types | ||
.get(dt.get_name().to_lowercase().as_str()) | ||
.expect("at this point we should have all types") // TODO: error msg |
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.
mention dt.get_name()
in the message
@@ -413,14 +484,27 @@ impl<'ink, 'cg> PouGenerator<'ink, 'cg> { | |||
m.get_name(), | |||
&index.get_associated_type(m.get_type_name())?, | |||
); | |||
|
|||
if let Some(block) = self.llvm.builder.get_insert_block() { | |||
debug.add_variable_declaration( |
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.
shall we also register variable declaration for the return variable? (aggregate and normal ones?)
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.
We already do, there's also a test for aggregate return types
We can maybe do a walkthrough for it
…etting-breakpoints-on-different-lines-in-code
…etting-breakpoints-on-different-lines-in-code
…ifferent-lines-in-code
Add support for line debugging
Debugging works on any assignment statement or function call
Loops and other conditions are still not supported
Variables are correctly associated during debugging.
Fixes: #534 #537