-
-
Notifications
You must be signed in to change notification settings - Fork 212
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: slightly better fuzzy scanning #115
base: dev
Are you sure you want to change the base?
Conversation
|
||
if (patternOpcode != null && patternOpcode.ordinal != originalOpcode.ordinal) { | ||
// reaching maximum threshold (0) means, | ||
// the pattern does not match to the current instructions | ||
if (threshold-- == 0) break | ||
|
||
// look ahead | ||
for (depth in 1 until threshold + 2) { |
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.
Is threshold + 2
arbitrary?
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.
Kind of, I wanted to make it so that at a minimum at least the next instruction is considered. If threshold is 0 at that point, you'll only enter that loop once since you start at 1 and would go 'until' 2 (exclusive).
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.
Before merging, can you rebase & test this PR against all patches for YouTube? |
da2b438
to
9ec720e
Compare
Can this be merged soonish? PR has been open for a long while now, waiting for @oSumAtrIX to review. |
If possible, please test this against a majority of the existing patches. |
646358d
to
c5de9e2
Compare
Hey, apologies for the delayed reply. I won't be able to look at this for quite some time. If someone else can pick it up that'd be great or else I think it'd be safer to close the PR. |
Well it can stay open just fine until we can test it. |
@oSumAtrIX This should be reopened. |
What's keeping this PR from being merged after almost a year? Is no one available from the team to test this change, if necessary? |
It has low priority because it is currently unnecessary as well as needs to be tested if its breaking |
64a5adf
to
77dbee3
Compare
I believe, if we want to properly fix and implement fuzzy scanning, something like https://github.com/intuit/fuzzy-matcher should be used instead. |
0fc5a47
to
ac1aff5
Compare
Closes #89