search

encounter / objdiff

A local diffing tool for decompilation projects
Apache License 2.0
124 stars 21 forks source link

Fix data tooltip panic #123

Closed SquareMan closed 1 month ago

SquareMan commented 1 month ago

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

Antidote commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Thanks for the fix!