Skip to content
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

Merged
merged 4 commits into from
Oct 15, 2024
Merged

Fix data tooltip panic #123

merged 4 commits into from
Oct 15, 2024

Conversation

SquareMan
Copy link
Contributor

@SquareMan SquareMan commented Oct 15, 2024

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

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.
@Antidote
Copy link

If I'm being 100% honest, I'd rather have a panic than a hacky "fix."
This ignores the underlying problem and a) fails clippy, and b) introduces ambiguity down the line.

@TheNathannator
Copy link

TheNathannator commented Oct 15, 2024

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.

@Antidote
Copy link

Antidote commented Oct 15, 2024

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.

@TheNathannator
Copy link

TheNathannator commented Oct 15, 2024

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.

@SquareMan
Copy link
Contributor Author

SquareMan commented Oct 15, 2024

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.

@encounter
Copy link
Owner

Thanks for the fix!

@encounter encounter merged commit 67b6331 into encounter:main Oct 15, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants