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

String concatenation visitor: fix for ArrayIndexOutOfBoundsException #427

Merged
merged 6 commits into from
Jan 12, 2019

Conversation

jpstotz
Copy link
Collaborator

@jpstotz jpstotz commented Jan 10, 2019

My log file was often very polluted by the string concatenation converter throwing ArrayIndexOutOfBoundsException when the StringBuilder chain contains calls not only to and append but also to toString() or any other method of StringBuilder that does not take an argument.

This pull request limit the chain to process only constructor class and calls to append.

@skylot
Copy link
Owner

skylot commented Jan 10, 2019

@jpstotz thank you for fix!
Actually, there are tons of issues with StringBuilder conversion. And I can't force myself to fix them, because this code so cryptic it is hard to understand how it works also it lack tests. I hope someday I will rewrite it completely :)
So can you add JUnit test to reproduce this issue? Or java sample method, so I can add test myself?

skylot and others added 3 commits January 12, 2019 14:01
test: simple JUnit test cases added for testing StringBuilder chain processing (chains that can be and that can't be simplified)
@jpstotz
Copy link
Collaborator Author

jpstotz commented Jan 12, 2019

@skylot You are right, JUnit test cases are a good thing. It turned out that I had a major misunderstanding regarding the visitor code which ended up it totally defect code created by it.

After creating the matching unit test I understand it much better. I think I have covered the major cases with the unit test. I wasn't sure where to put the unit test (which package). Therefore feel free to move it where you think it belongs to.

@@ -201,7 +204,15 @@ private static InsnNode convertInvoke(MethodNode mth, InsnNode insn) {
}

for (; argInd < len; argInd++) { // Add the .append(xxx) arg string to concat
concatInsn.addArg(chain.get(argInd).getArg(1));
InsnNode node = chain.get(argInd);
MethodInfo method = ((CallMthInterface) node).getCallMth();
Copy link
Owner

Choose a reason for hiding this comment

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

Add instanceof check before the cast, it will make code much safer.
And next time instead of merging just rebase your commits on latest master, also fix for the previous commit is better to squash together, so you will get 2 nice commits on top of master :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The missing instanceof check is on purpose. If we encounter such a case most likely can't simplify the chain. It will then raise an Exception so that anybody may notice it and we can take a closer look onto it. Therefore the effect is the same the only difference is that without the instanceof check nothing is logged.

Rebase: According what I have read you should not do this once you have pushed your commits to a public repo. And as the branch this PR bases on is public...

Therefore I will keep on using git as I do - at least most of the times it does what I want.
If they develop such a complex version control system they should make it more usable and intuitive to use.

@skylot skylot merged commit 72b2663 into skylot:master Jan 12, 2019
@skylot
Copy link
Owner

skylot commented Feb 12, 2019

🎉 This PR is included in version 0.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants