-
Notifications
You must be signed in to change notification settings - Fork 601
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
New performance rule for Appendable appending CharSequence #704
New performance rule for Appendable appending CharSequence #704
Comments
I can take this. I'm looking for instances of this occurrence. Any pointers? |
You might want to take a look at the |
Thanks for this @ThrawnCA I was looking at the code. To summarize the requirement - we are going to fix charSequence.toString() pattern in the spotbugs repository, right? If so, running the plugin on this repo and looking for ISB_TOSTRING_APPENDING did not yield any such results. Am I doing it the right way or is there any other way to do it? I'm working on this repo for the first time. Thanks. |
Well, that's certainly possible, but not what I understood the original request to be. IIUC @blindspeed90 was suggesting that SpotBugs should include a detector for this pattern, and since you put your hand up, I suggested ISB_TOSTRING_APPENDING as an example of a similar detector that you could base it on. |
Great.. Thanks for this info.. I am going to proceed in this way now. What you understood makes more sense :) I have done initial research and am thinking it would be on the lines of InefficientIndexOf, more than ISB_TOSTRING_APPENDING, as it belongs to the same repository (spotbugs). Also, what do you think is the best way to test the code I've written on my local machine, w.r.t this? I'm presuming unit tests, but I do not find tests in the detect package that relate to actually testing bugs of specific types. |
Just to make sure we've covered all the bases, there are two signatures in Appendable: append(CharSequence csq) and the object type to check shouldn't just be an Appendable but an instanceof Appendable so for example all of these will match: writer.append(stringBuilder) |
@sarankumarv Well, for starters, make sure that you add a sample of the problem in That done - there isn't great coverage of the existing detectors, but you can take a look at |
@blindspeed90 Thanks for the clarification! The gist - So, when any instance of Appendable, calls append(CharSequence) or append(CharSequence, int, int), we should trigger the rule if toString() is called on CharSequence or its implementations. |
@ThrawnCA Thanks! These helped. I'm trying to run the code and see how exactly the rules are triggered, taking InefficientIndexOf as an example. I'm taking code pieces and thus, analyzing the byte code of charSequence.toString() and how actually InvokeVirtual is called, etc. I'll get back to you in case I need anything. :) |
I was looking for a good first issue and came across this one. As suggested by @ThrawnCA , have been able to fix this on similar lines as fb-contrib bug detector ISB_TOSTRING_APPENDING in a clone of the fb-contrib repository. It makes sense to submit pull request to fb-contrib. |
Go ahead and add to fb-contrib. If you include a link to this issue, Github will make it visible here. |
Look for the pattern appendable.append(charSequence.toString()) to bypass the interim string and just do appendable.append(charSequence).
The text was updated successfully, but these errors were encountered: