Otavio,
Just skimmed through your changes. It looks good. But there are some things we
can make a little bit better though. IMO, it's not always a performance that
matters (looking around to see if Alexey Shipilev is somewhere near) but
readability. It's good to estimate performance requirements of a code before
making a decision of whether to sacrifice readability for performance or not. I
noticed there are some methods that are screaming for readability improvements.
Namely, different versions of joining strings through a separator. You can find
them in following methods in your changeset:
sun.security.acl.AclEntryImpl::toString
sun.security.ssl.HandshakeMessage.CertificateRequest::print
javax.swing.MultiUIDefaults::toString (one of the most bizarre one)
sun.security.provider.PolicyFile::expandSelf
java.security.ProtectionDomain::toString
javax.management.relation.Role::toString
sun.security.ssl.SupportedEllipticCurvesExtension::toString
sun.tools.jstat.SyntaxException::SyntaxException
Unfortunately, neither java.util.StringJoiner nor String.join have perfect (but
who has?) APIs. So it's up to us to figure out the best way of joining
elements. So when the power of those guys is not enough, we can go this way (or
maybe some else):
Iterable<?> elements = ...
StringJoiner j = new StringJoiner(", ");
elements.forEach(e -> j.add(e.toString()));
or
Collection<?> elements = ...
String i = elements.stream()
.map(Object::toString)
.collect(Collectors.joining(", "));
Other than that:
1. It looks like some StringBuffers are still lurking out there. We should
change them to StringBuilders, *only if* we can prove they are not exposed to
more than one thread:
--- dev/jdk/src/share/classes/javax/sound/sampled/AudioFileFormat.java
(revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/javax/sound/sampled/AudioFileFormat.java
(revision 10452+:8e501e6bbf1f+)
@@ -272,7 +272,7 @@
@Override
public String toString() {
- StringBuffer sb = new StringBuffer();
+ StringBuilder sb = new StringBuilder();
//$$fb2002-11-01: fix for 4672864: AudioFileFormat.toString() throws
unexpected NullPointerException
if (type != null) {
---
dev/jdk/src/share/classes/com/sun/tools/hat/internal/model/JavaValueArray.java
(revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++
dev/jdk/src/share/classes/com/sun/tools/hat/internal/model/JavaValueArray.java
(revision 10452+:8e501e6bbf1f+)
@@ -346,12 +346,12 @@
public String valueString(boolean bigLimit) {
// Char arrays deserve special treatment
- StringBuffer result;
+ StringBuilder result;
byte[] value = getValue();
int max = value.length;
byte elementSignature = getElementType();
if (elementSignature == 'C') {
- result = new StringBuffer();
+ result = new StringBuilder();
for (int i = 0; i < value.length; ) {
char val = charAt(i, value);
result.append(val);
@@ -362,7 +362,7 @@
if (bigLimit) {
limit = 1000;
}
- result = new StringBuffer("{");
+ result = new StringBuilder("{");
int num = 0;
for (int i = 0; i < value.length; ) {
if (num > 0) {
--- dev/jdk/src/share/classes/javax/swing/GroupLayout.java (revision
10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/javax/swing/GroupLayout.java (revision
10452+:8e501e6bbf1f+)
@@ -1213,7 +1213,7 @@
registerComponents(horizontalGroup, HORIZONTAL);
registerComponents(verticalGroup, VERTICAL);
}
- StringBuffer buffer = new StringBuffer();
+ StringBuilder buffer = new StringBuilder();
buffer.append("HORIZONTAL\n");
createSpringDescription(buffer, horizontalGroup, " ", HORIZONTAL);
buffer.append("\nVERTICAL\n");
@@ -1221,7 +1221,7 @@
return buffer.toString();
}
- private void createSpringDescription(StringBuffer buffer, Spring spring,
+ private void createSpringDescription(StringBuilder buffer, Spring spring,
String indent, int axis) {
String origin = "";
String padding = "";
--- dev/jdk/src/share/classes/sun/net/spi/nameservice/dns/DNSNameService.java
(revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/sun/net/spi/nameservice/dns/DNSNameService.java
(revision 10452+:8e501e6bbf1f+)
@@ -463,7 +463,7 @@
// ---------
- private static void appendIfLiteralAddress(String addr, StringBuffer sb) {
+ private static void appendIfLiteralAddress(String addr, StringBuilder sb) {
if (IPAddressUtil.isIPv4LiteralAddress(addr)) {
sb.append("dns://").append(addr).append(' ');
} else {
@@ -478,10 +478,8 @@
* corresponding to the supplied List of nameservers.
*/
private static String createProviderURL(List<String> nsList) {
- StringBuffer sb = new StringBuffer();
- for (String s: nsList) {
- appendIfLiteralAddress(s, sb);
- }
+ StringBuilder sb = new StringBuilder();
+ nsList.forEach(s -> appendIfLiteralAddress(s, sb));
return sb.toString();
}
@@ -491,7 +489,7 @@
* contained in the provided str.
*/
private static String createProviderURL(String str) {
- StringBuffer sb = new StringBuffer();
+ StringBuilder sb = new StringBuilder();
StringTokenizer st = new StringTokenizer(str, ",");
while (st.hasMoreTokens()) {
appendIfLiteralAddress(st.nextToken(), sb);
2. Also there are some minor issues with line width and readability:
--- dev/jdk/src/share/classes/java/security/cert/CertPath.java (revision
10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/java/security/cert/CertPath.java (revision
10452+:8e501e6bbf1f+)
@@ -224,16 +224,16 @@
Iterator<? extends Certificate> stringIterator =
getCertificates().iterator();
- sb.append('\n').append(type).append(" Cert Path: length =
").append(getCertificates().size()).append(".\n");
- sb.append("[\n");
+ sb.append('\n').append(type).append(" Cert Path: length = ")
+ .append(getCertificates().size()).append(".\n").append("[\n");
int i = 1;
while (stringIterator.hasNext()) {
- sb.append("==========================================" +
"===============Certificate ")
- .append(i).append(" start.\n");
+ sb.append("==========================================")
+ .append("===============Certificate ").append(i).append("
start.\n");
Certificate stringCert = stringIterator.next();
- sb.append(stringCert.toString());
- sb.append("\n========================================" +
"=================Certificate ")
- .append(i).append(" end.\n\n\n");
+ sb.append(stringCert.toString()).append('\n');
+ sb.append("========================================")
+ .append("=================Certificate ").append(i).append("
end.\n\n\n");
i++;
}
--- dev/jdk/src/share/classes/javax/swing/JColorChooser.java (revision
10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/javax/swing/JColorChooser.java (revision
10452+:8e501e6bbf1f+)
@@ -543,7 +543,7 @@
* @return a string representation of this <code>JColorChooser</code>
*/
protected String paramString() {
- StringBuilder chooserPanelsString = new StringBuilder("");
+ StringBuilder chooserPanelsString = new StringBuilder();
for (int i=0; i<chooserPanels.length; i++) {
chooserPanelsString.append('[').append(chooserPanels[i].toString()).append(']');
}
3. Btw, I wonder what "try ... catch" is doing here. Haven't we checked
enum_.hasMoreElements() is true? (I guess it's a question to the security team).
--- dev/jdk/src/share/classes/java/security/PermissionCollection.java
(revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/java/security/PermissionCollection.java
(revision 10452+:8e501e6bbf1f+)
@@ -181,13 +181,9 @@
StringBuilder sb = new StringBuilder();
sb.append(super.toString()).append(" (\n");
while (enum_.hasMoreElements()) {
- try {
- sb.append(' ');
- sb.append(enum_.nextElement().toString());
- sb.append('\n');
+ sb.append(' ');
+ sb.append(enum_.nextElement().toString());
+ sb.append('\n');
- } catch (NoSuchElementException e){
- // ignore
- }
}
sb.append(")\n");
return sb.toString();
4. This method looks dead (not used):
---
dev/jdk/src/share/classes/com/sun/tools/example/debug/gui/ContextManager.java
(revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++
dev/jdk/src/share/classes/com/sun/tools/example/debug/gui/ContextManager.java
(revision 10452+:8e501e6bbf1f+)
@@ -348,15 +348,4 @@
return javaArgs;
}
}
-
- private String appendPath(String path1, String path2) {
- if (path1 == null || path1.length() == 0) {
- return path2 == null ? "." : path2;
- } else if (path2 == null || path2.length() == 0) {
- return path1;
- } else {
- return path1 + File.pathSeparator + path2;
- }
- }
-
}
-Pavel
