-
Notifications
You must be signed in to change notification settings - Fork 21
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
Fix data tooltip panic #123
Conversation
Prevents panicing when attempting to display the data tooltip for a symbol that is too large by just using as many bytes as needed from the begging of the symbol.
If I'm being 100% honest, I'd rather have a panic than a hacky "fix." |
For a GUI program, I'd much prefer if the error was handled gracefully, rather than having it inexplicably crash. The better option IMO would be to display an error message in place of the tooltip on failure. |
I agree, the PR doesn't do anything but hide the problem, it doesn't address it in any meaningful way, which is what I meant by preferring it to panic. |
Be sure to express that intent clearly then lol, it's rather dangerous to let that message be taken at face value as code feedback. |
I've addressed the comments here and though I disagree with some things, I've made some changes and think that we can all agree that this new fix is better than a panic. Now if the symbol's size doesn't match the expected size exactly, it will just return None causing the tooltip to show no value for that symbol. I opened #124 to track the improvement for this feature to still allow for showing some useful data in these cases. |
Thanks for the fix! |
Prevents panicing when attempting to display the data tooltip for a symbol that is too large by just using as many bytes as needed from the begging of the symbol.
This is a quick fix for the data tooltip panic that matches my original intention behind the code.
As discussed in discord we have a few different options when the symbol's size is larger than the data type associated with the instruction used.
A) Return None and have no tooltip
B) Interpret the value starting from the beginning of the symbol
C) Interpret the value starting from the relative offset of the symbol
D) Show the value of the entire symbol interpreted as a larger data type
This implements B,but I think C would be better. That needs some additional work though to make happen. IMO A and D are just bad.EDIT: This PR as merged implements A