-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[java] Fix issue #1736 and issue #2207 #2558
Conversation
Generated by 🚫 Danger |
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.
Hi @YYoungC , thanks for your pull request.
I'm afraid, I can't accept it as is.
Please have a look at my comments and create a unit test cases to proof, that we actually fixed the issues, we wanted to fix. Adding a unit test is easy:
- General documentation about rule testing: https://pmd.github.io/latest/pmd_userdocs_extending_testing.html
- A new test case needs to be added to either https://github.com/pmd/pmd/blob/master/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/UseStringBufferForStringAppends.xml and https://github.com/pmd/pmd/blob/master/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/performance/xml/AvoidInstantiatingObjectsInLoops.xml
- For the sample test code, look at the sample of [java] UseStringBufferForStringAppends: False positive if only one concatenation #1736 and [java] AvoidInstantiatingObjectsInLoops: False positive - should not flag objects when assigned to lists/arrays #2207
...ava/net/sourceforge/pmd/lang/java/rule/performance/AvoidInstantiatingObjectsInLoopsRule.java
Outdated
Show resolved
Hide resolved
...ava/net/sourceforge/pmd/lang/java/rule/performance/AvoidInstantiatingObjectsInLoopsRule.java
Outdated
Show resolved
Hide resolved
...ava/net/sourceforge/pmd/lang/java/rule/performance/AvoidInstantiatingObjectsInLoopsTest.java
Outdated
Show resolved
Hide resolved
...ava/net/sourceforge/pmd/lang/java/rule/performance/AvoidInstantiatingObjectsInLoopsTest.java
Outdated
Show resolved
Hide resolved
...java/net/sourceforge/pmd/lang/java/rule/performance/UseStringBufferForStringAppendsRule.java
Outdated
Show resolved
Hide resolved
...java/net/sourceforge/pmd/lang/java/rule/performance/UseStringBufferForStringAppendsTest.java
Outdated
Show resolved
Hide resolved
There are still many new violations, but these are ok - they have been actually false-negative, as this rule was not using rule chain previously but still short-circuited the visitor by returning data. Now all AllocationExpressions are visisted, even nested ones in a anonymous class allocation expression, when the anonymous class is not within a loop. I'll merge that now. |
Describe the PR
Related issues
Ready?
./mvnw clean verify
passes (checked automatically by travis)