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

[jackpot] DefaultRuleUtilities::referencedIn fix for single variable matching #3540

Merged
merged 1 commit into from Feb 1, 2022

Conversation

mbien
Copy link
Member

@mbien mbien commented Jan 29, 2022

referencedIn(v, in) did not match if in was a single variable.

example:

for(int $i = $from; $i < $to; $i++) {
    $array[$i] = $n;
} 
:: !referencedIn($i, $n) && !matchesAny($n, "$i") /*&& more()*/ =>
java.util.Arrays.fill($array, $n);
;;

needed an extra !matchesAny($n, "$i") to make sure $n is not actually equal to $i. (i+1 would be matched by referencedIn but not i). After this PR && !matchesAny is no longer needed.

  • added a small optimization which adds a fast path to the recursion if a result has been already determined.
  • couldn't find tests for referencedIn so I added some.

btw the in-editor test integration for rule files is super cool

@mbien mbien added the Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) label Jan 29, 2022
Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am amazed by your contributions to Jackpot, Michael. It is great to see someone else than @jlahoda taking care of the tool!

Writing a test as well as changing code is always good thing. I didn't even know we have referencedIn condition. Shouldn't we also improve the documentation to give a nice example of this (and possibly other) conditions?

@mbien
Copy link
Member Author

mbien commented Jan 31, 2022

@JaroslavTulach I do what i can but a lot of this is still black magic to me :)

I am going to rebase this PR for delivery just in case it can be still integrated. This looks fairly low risk to me (I don't think the utility functions are even used in netbeans itself).

@mbien mbien changed the base branch from master to delivery January 31, 2022 09:58
@mbien mbien added this to the NB13 milestone Jan 31, 2022
@neilcsmith-net
Copy link
Member

This looks fairly low risk to me (I don't think the utility functions are even used in netbeans itself).

Not sure how to assess the latter point against bug criteria then, so will take your word on the first point and sneak it in rc3! 😄 Will merge tomorrow, give @jlahoda time to comment if needed.

@mbien mbien added the kind:bug Bug report or fix label Jan 31, 2022
@neilcsmith-net neilcsmith-net merged commit fd384f0 into apache:delivery Feb 1, 2022
@jlahoda
Copy link
Contributor

jlahoda commented Feb 2, 2022

Looks great, BTW!

@mbien mbien added the hints label Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hints Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) kind:bug Bug report or fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants