Skip to content

[java] New Performance Rule AvoidConcatInLoop#2575

Closed
adangel wants to merge 5 commits into
pmd:masterfrom
adangel:perf-AvoidConcatInLoop
Closed

[java] New Performance Rule AvoidConcatInLoop#2575
adangel wants to merge 5 commits into
pmd:masterfrom
adangel:perf-AvoidConcatInLoop

Conversation

@adangel

@adangel adangel commented Jun 13, 2020

Copy link
Copy Markdown
Member

Describe the PR

This is the rule "AvoidConcatInLoop" split from original PR #1932 .

Related issues

Ready?

  • It seems, that the rulechain for xpath 2.0 impl makes the rule fail. The xpath (//A | //B)//C somehow only returns nodes of type A or B, but misses the C part... That's why this PR is WIP.
  • possible false positive: https://github.com/pmd/pmd/pull/1932/files#r307543200
  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by travis)
  • Added (in-code) documentation (if needed)

jborgers and others added 5 commits June 13, 2020 19:15
@adangel adangel added a:new-rule Proposal to add a new built-in rule is:WIP For PRs that are not fully ready, or issues that are actively being tackled labels Jun 13, 2020
@oowekyala

Copy link
Copy Markdown
Member

This rule shares the same justification as InefficientStringBuffering (like the other rule AvoidConcatInAppend in #1932). Maybe these should be merged?

@adangel

adangel commented Jun 18, 2020

Copy link
Copy Markdown
Member Author

This rule shares the same justification as InefficientStringBuffering (like the other rule AvoidConcatInAppend in #1932). Maybe these should be merged?

In InefficientStringBuffering, you already use StringBuilder/Buffer, but not efficiently. In AvoidConcatInLoop you don't use StringBuilder yet, just ordinary string concats. But special case: in a loop. We have another rule UseStringBufferForStringAppends where this one would fit... It would be then a false-negative, if UseStringBufferForStringAppends wouldn't detect string appends (aka concats) inside loops...

@adangel

adangel commented Jun 18, 2020

Copy link
Copy Markdown
Member Author

Most cases were indeed already handled by the existing rule, so closing this PR in favor of #2600.

@adangel adangel closed this Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:new-rule Proposal to add a new built-in rule is:WIP For PRs that are not fully ready, or issues that are actively being tackled

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants