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

New performance rule for Appendable appending CharSequence #704

Open
blindspeed90 opened this issue Jul 20, 2018 · 11 comments · May be fixed by mebigfatguy/fb-contrib#391
Open

New performance rule for Appendable appending CharSequence #704

blindspeed90 opened this issue Jul 20, 2018 · 11 comments · May be fixed by mebigfatguy/fb-contrib#391

Comments

@blindspeed90
Copy link

Look for the pattern appendable.append(charSequence.toString()) to bypass the interim string and just do appendable.append(charSequence).

@s18v
Copy link

s18v commented Jul 23, 2018

I can take this. I'm looking for instances of this occurrence. Any pointers?

@ThrawnCA
Copy link
Contributor

@s18v
Copy link

s18v commented Jul 24, 2018

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.

@ThrawnCA
Copy link
Contributor

we are going to fix charSequence.toString() pattern in the spotbugs repository, right?

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.

@s18v
Copy link

s18v commented Jul 25, 2018

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.

@blindspeed90
Copy link
Author

Just to make sure we've covered all the bases, there are two signatures in Appendable:

append(CharSequence csq)
append(CharSequence csq, int start, int end)

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)
stringBuilder.append(string)
...

@ThrawnCA
Copy link
Contributor

@sarankumarv Well, for starters, make sure that you add a sample of the problem in spotbugsTestCases/src/java/

That done - there isn't great coverage of the existing detectors, but you can take a look at test-harness/src/main/java/edu/umd/cs/findbugs/test/SpotBugsRule.java and test-harness-core/src/main/java/edu/umd/cs/findbugs/test/AnalysisRunner.java for help.

@s18v
Copy link

s18v commented Jul 26, 2018

@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.

@s18v
Copy link

s18v commented Jul 26, 2018

@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. :)

@manishaghaisas
Copy link

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.
Should I go ahead and open a new issue in fb-contrib? Is there a way to link to this issue?

@ThrawnCA
Copy link
Contributor

ThrawnCA commented Mar 4, 2020

Go ahead and add to fb-contrib. If you include a link to this issue, Github will make it visible here.

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

Successfully merging a pull request may close this issue.

5 participants