-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
PoC for the issue #980 #1449
base: master
Are you sure you want to change the base?
PoC for the issue #980 #1449
Conversation
struct Approximate { | ||
std::endian endian = std::endian::native; | ||
bool aligned = true; | ||
bool ignore_zeroes = true; |
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.
Use camelCase
reader.seek(searchRegion.getStartAddress()); | ||
reader.setEndAddress(searchRegion.getEndAddress()); | ||
|
||
using FloatingType = ViewFind::SearchSettings::Approximate::Type; |
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.
FloatingPointType
const size_t size = settings.type == FloatingType::F32 ? sizeof(float) : sizeof(double); | ||
const auto advance = settings.aligned ? size : 1; | ||
|
||
std::variant<float, double> floating_var; |
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.
Use a more descriptive name for this. What does the variable do?
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.
I've adapted the code for the Numeric search as we may want to add more floating types here. Please note that you used a return value of parseNumericValueInput
(the middle one, min
), which I thought was a dirty trick to acquire an instance of std::variant. So I made my own here.
|
||
std::variant<float, double> floating_var; | ||
if (settings.type == FloatingType::F32) { | ||
floating_var = 0.F; |
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.
0.0F
I'd probably also be explicit here and use float(0)
and double(0)
to make sure the right alternative is being set in the variant
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.
The literal suffix "f" (or "F" which Clang-tidy wanted me to use) is used to turn doubles into floats. It's what forces std::variant to accept the desired type.
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.
I know the suffix makes it a float but explicitly writing out the type is better in my opinion
@@ -800,6 +860,53 @@ namespace hex::plugin::builtin { | |||
|
|||
ImGui::EndTabItem(); | |||
} | |||
if (ImGui::BeginTabItem("hex.builtin.view.find.approximate"_lang)) { |
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.
Does it make sense to separate this out to a separate search tool instead of merging it into the numeric find tool?
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.
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.
I didn't compile it yet but if it's just a few extra options, they could be hidden unless a floating point type is selected.
I'm fine with having it as a separate tool too but functionality-wise it would fit in the Numeric search tool
Thank you! I really like the idea of the tool |
Any news for this ? |
This very well may be a new PR after considering the possibilities and testing... This doesn't implement all the things I wanted to, but I could add more commits here to see how it can go.