[PATCH] 0012 - Fixes for Coverity Issues of type StringBuffer_Concatenation, Wrong_Map_Iterator, No_Equals_Method , Reverse_INull for DogTag 10
Please find attached PATCH 12 - with fixes for issues in Coverity of types StringBuffer_Concatenation, Wrong_Map_Iterator, No_Equals_Method , Reverse_INull for review. Build passed the Smoke Test. Regards, Abhishek Koneru
On 6/13/2012 4:31 PM, Abhishek Koneru wrote:
Please find attached PATCH 12 - with fixes for issues in Coverity of types StringBuffer_Concatenation, Wrong_Map_Iterator, No_Equals_Method , Reverse_INull for review.
Some issues: 1. There's a formatting issue in CMSCRLExtensions.java:586. 2. In CRLIssuingPoint.java:2712-2737 the String concatenations are replaced with StringBuffer. This is fine and not a big deal but it can be done more efficiently. See http://kaioa.com/node/59 and http://javarevisited.blogspot.com/2011/07/string-vs-stringbuffer-vs-stringbu.... In this code the only thread using the StringBuffer is the current thread itself, so there's no concurrency issue. So the StringBuffer can be replaced with StringBuilder. Every time you do a "+" operation on Strings, the JVM will create a StringBuilder, append the Strings, then generate a new String that contains the concatenated text. So instead of this: splitTimes.append( "," + Long.toString(deltaTime) + "," + Long.toString(crlTime) + "," + Long.toString(totalTime) + ")"); you can reuse the same StringBuilder object like this: splitTimes.append(",").append(deltaTime).append(",").append(crlTime) .append(",").append(totalTime).append(")"); Notice the Long.toString() is not necessary because there are separate append() methods for each primitive type. Also with autoboxing the primitive types will be converted automatically to objects, so the code in lines 2722, 2729-2738 can be simplified like this: splitTimes.append(mSplits[i]); new Object[] { getId(), getCRLNumber(), getLastUpdate(), getNextUpdate(), mCRLSize, totalTime, crlTime, "" + deltaTime + splitTimes } 3. In CRLIssuingPoint.java:3098-3105,3151 whenever you remove the { } surrounding a code block you should fix the indentation. 4. In NameValuePairs.java:47 you can use StringBuilder too. In line 51 it's better to use separate append() for each String component. 5. In SimpleProperties.java you should use Eclipse to generate hashCode() too, it goes together with equals(). 6. In Auditor.java the StringBuilder can be created once at line 99, then the subsequent code will append each String component separately. 7. In ACL.java:153 you can use StringBuilder. In lines 159 and 163 it's not necessary to call toString() explicitly. 8. In ExtDataHashtable.java:59 I think you can actually remove the putAll() because it's identical to the one in the super class. 9. In RequestTest.java you should add hashCode() as well. 10. Same thing in CMCResponse.java:125-130, use StringBuilder, separate appends, no explicit toString(). 11. Same thing in PrettyPrintCert.java:89,200,221, use StringBuilder, separate appends, no explicit toString(). 12. Add hashCode() to the following files: - CMSProperties.java - PKCS10Attributes.java - DSAPrivateKey.java - DSAPublicKey.java - RSAPublicKey.java - AlgIdDSA.java - CRLExtensions.java - CertificateExtensions.java - Extensions.java -- Endi S. Dewata
On Thu, 2012-06-14 at 19:11 -0500, Endi Sukma Dewata wrote:
On 6/13/2012 4:31 PM, Abhishek Koneru wrote:
Please find attached PATCH 12 - with fixes for issues in Coverity of types StringBuffer_Concatenation, Wrong_Map_Iterator, No_Equals_Method , Reverse_INull for review.
Some issues:
1. There's a formatting issue in CMSCRLExtensions.java:586.
2. In CRLIssuingPoint.java:2712-2737 the String concatenations are replaced with StringBuffer. This is fine and not a big deal but it can be done more efficiently. See http://kaioa.com/node/59 and http://javarevisited.blogspot.com/2011/07/string-vs-stringbuffer-vs-stringbu....
In this code the only thread using the StringBuffer is the current thread itself, so there's no concurrency issue. So the StringBuffer can be replaced with StringBuilder.
Every time you do a "+" operation on Strings, the JVM will create a StringBuilder, append the Strings, then generate a new String that contains the concatenated text. So instead of this:
splitTimes.append( "," + Long.toString(deltaTime) + "," + Long.toString(crlTime) + "," + Long.toString(totalTime) + ")");
you can reuse the same StringBuilder object like this:
splitTimes.append(",").append(deltaTime).append(",").append(crlTime) .append(",").append(totalTime).append(")");
While I appreciate that the above formulation is more efficient, there is a drawback in terms of readability. This following formulation may be a little less efficient but is much more readable: splitTimes.append("," + deltaTime + "," + crlTime + "," + totalTime + ")"); I suggest that we not sacrifice readability for efficiency until we have profiling data in hand. I suspect that this little inefficiency will be the very least of our performance issues.
Notice the Long.toString() is not necessary because there are separate append() methods for each primitive type. Also with autoboxing the primitive types will be converted automatically to objects, so the code in lines 2722, 2729-2738 can be simplified like this:
splitTimes.append(mSplits[i]);
new Object[] { getId(), getCRLNumber(), getLastUpdate(), getNextUpdate(), mCRLSize, totalTime, crlTime, "" + deltaTime + splitTimes }
3. In CRLIssuingPoint.java:3098-3105,3151 whenever you remove the { } surrounding a code block you should fix the indentation.
4. In NameValuePairs.java:47 you can use StringBuilder too. In line 51 it's better to use separate append() for each String component.
5. In SimpleProperties.java you should use Eclipse to generate hashCode() too, it goes together with equals().
6. In Auditor.java the StringBuilder can be created once at line 99, then the subsequent code will append each String component separately.
7. In ACL.java:153 you can use StringBuilder. In lines 159 and 163 it's not necessary to call toString() explicitly.
8. In ExtDataHashtable.java:59 I think you can actually remove the putAll() because it's identical to the one in the super class.
9. In RequestTest.java you should add hashCode() as well.
10. Same thing in CMCResponse.java:125-130, use StringBuilder, separate appends, no explicit toString().
11. Same thing in PrettyPrintCert.java:89,200,221, use StringBuilder, separate appends, no explicit toString().
12. Add hashCode() to the following files: - CMSProperties.java - PKCS10Attributes.java - DSAPrivateKey.java - DSAPublicKey.java - RSAPublicKey.java - AlgIdDSA.java - CRLExtensions.java - CertificateExtensions.java - Extensions.java
On 6/14/2012 7:36 PM, Ade Lee wrote:
Every time you do a "+" operation on Strings, the JVM will create a StringBuilder, append the Strings, then generate a new String that contains the concatenated text. So instead of this:
splitTimes.append( "," + Long.toString(deltaTime) + "," + Long.toString(crlTime) + "," + Long.toString(totalTime) + ")");
you can reuse the same StringBuilder object like this:
splitTimes.append(",").append(deltaTime).append(",").append(crlTime) .append(",").append(totalTime).append(")");
While I appreciate that the above formulation is more efficient, there is a drawback in terms of readability. This following formulation may be a little less efficient but is much more readable:
splitTimes.append("," + deltaTime + "," + crlTime + "," + totalTime + ")");
I suggest that we not sacrifice readability for efficiency until we have profiling data in hand. I suspect that this little inefficiency will be the very least of our performance issues.
Agreed. For better readability we can use this: splitTimes.append( String.format(",%d,%d,%d)", deltaTime, crlTime, totalTime)); -- Endi S. Dewata
Please find attached the patch for fixes for the issues in PATCH 12 for review and commit. Regards, Abhishek Koneru On Thu, 2012-06-14 at 19:11 -0500, Endi Sukma Dewata wrote:
On 6/13/2012 4:31 PM, Abhishek Koneru wrote:
Please find attached PATCH 12 - with fixes for issues in Coverity of types StringBuffer_Concatenation, Wrong_Map_Iterator, No_Equals_Method , Reverse_INull for review.
Some issues:
1. There's a formatting issue in CMSCRLExtensions.java:586.
2. In CRLIssuingPoint.java:2712-2737 the String concatenations are replaced with StringBuffer. This is fine and not a big deal but it can be done more efficiently. See http://kaioa.com/node/59 and http://javarevisited.blogspot.com/2011/07/string-vs-stringbuffer-vs-stringbu....
In this code the only thread using the StringBuffer is the current thread itself, so there's no concurrency issue. So the StringBuffer can be replaced with StringBuilder.
Every time you do a "+" operation on Strings, the JVM will create a StringBuilder, append the Strings, then generate a new String that contains the concatenated text. So instead of this:
splitTimes.append( "," + Long.toString(deltaTime) + "," + Long.toString(crlTime) + "," + Long.toString(totalTime) + ")");
you can reuse the same StringBuilder object like this:
splitTimes.append(",").append(deltaTime).append(",").append(crlTime) .append(",").append(totalTime).append(")");
Notice the Long.toString() is not necessary because there are separate append() methods for each primitive type. Also with autoboxing the primitive types will be converted automatically to objects, so the code in lines 2722, 2729-2738 can be simplified like this:
splitTimes.append(mSplits[i]);
new Object[] { getId(), getCRLNumber(), getLastUpdate(), getNextUpdate(), mCRLSize, totalTime, crlTime, "" + deltaTime + splitTimes }
3. In CRLIssuingPoint.java:3098-3105,3151 whenever you remove the { } surrounding a code block you should fix the indentation.
4. In NameValuePairs.java:47 you can use StringBuilder too. In line 51 it's better to use separate append() for each String component.
5. In SimpleProperties.java you should use Eclipse to generate hashCode() too, it goes together with equals().
6. In Auditor.java the StringBuilder can be created once at line 99, then the subsequent code will append each String component separately.
7. In ACL.java:153 you can use StringBuilder. In lines 159 and 163 it's not necessary to call toString() explicitly.
8. In ExtDataHashtable.java:59 I think you can actually remove the putAll() because it's identical to the one in the super class.
9. In RequestTest.java you should add hashCode() as well.
10. Same thing in CMCResponse.java:125-130, use StringBuilder, separate appends, no explicit toString().
11. Same thing in PrettyPrintCert.java:89,200,221, use StringBuilder, separate appends, no explicit toString().
12. Add hashCode() to the following files: - CMSProperties.java - PKCS10Attributes.java - DSAPrivateKey.java - DSAPublicKey.java - RSAPublicKey.java - AlgIdDSA.java - CRLExtensions.java - CertificateExtensions.java - Extensions.java
On 6/15/2012 12:56 PM, Abhishek Koneru wrote:
Please find attached the patch for fixes for the issues in PATCH 12 for review and commit.
ACK. Merged and pushed to master. -- Endi S. Dewata
participants (3)
-
Abhishek Koneru -
Ade Lee -
Endi Sukma Dewata