Add getChars to CharSequence (original) (raw)

Peter Levart peter.levart at gmail.com
Mon May 20 09:12:46 UTC 2013

On 05/09/2013 02:30 AM, Mike Duigou wrote:

Hi Martin;

Some notes from a non-exhaustive (ran out of time before dinner) review. Mike AbstractStringBuilder:: - The impls like insert(int dstOffset, CharSequence s) makes me slightly uneasy because the private value field escapes to an unknown class. Who knows what evil (or foolishness) could result?

Hi Martin,

Another consequence of the change should be considered carefully. Currently methods like append/insert taking String, StringBuffer, StringBuilder or CharSequence arguments are a mix of implementations where one set of them are invoking .getChars() on the argument, and other are iterating explicitly over the characters of the argument (those taking CharSequence and not doing delegation to more specific ones). The proposed patch makes use of CharSequence.getChars() in the later ones too. What changes? Synchronization. When StringBuffer is passed into such method and synchronized .getChars() is invoked, another object monitor is acquired where it was not previously. This could be considered more correct, since the view of the StringBuffer argument is locked for the time the characters are obtained from it, (not entirely correct though, see below), but could this provoke a deadlock in a situation where it didn't before? Use-cases that first come to mind are cases that are plagued with races in current implementation anyway, so they are bugs already. But for valid cases, the additional synchronization is still not enough.

For example, the proposed changed implementation of the following method in AbstractStringBuilder:

1052 public AbstractStringBuilder insert(int dstOffset, CharSequence s) { 1053 int tail = count - dstOffset; 1054 if ((dstOffset | tail) < 0) 1055 throw new StringIndexOutOfBoundsException(dstOffset); 1056 if (s == null) 1057 s = "null"; 1058 int len = s.length(); 1059 ensureCapacityInternal(count + len); 1060 System.arraycopy(value, dstOffset, value, dstOffset + len, tail); 1061 s.getChars(value, dstOffset); 1062 count += len; 1063 return this; 1064 }

...is still not entirely correct. If it is invoked with a StringBuffer argument when a concurrent thread is modifying it, the s.length() can be different from the actual length at the time the s.getChars() is called and consequently, IndexOutOfBoundsException can be thrown or characters can be overwritten unintentionally.

Regards, Peter

Direct-X-Buffer.java:: - +#if[rw] public boolean isDirect() : Why would this be conditionalized with rw? Heap-X-Buffer.java:: - protected -> private int ix(int i) : Is Alan OK with this change. I've mostly avoided these templates. :-) X-Buffer.java.template:: - toString() could use the JavaLangAccess.newUnsafeString() constructor! - I prefer your formatting of "return bigEndian ?". test/.../GetChars:: - Great to see you've already adopted using TestNG for JTReg tests! - ArraysCharSequence.hashCode() could have been Arrays.hashcode(chars) or not implemented at all. - Could use a @DataProivder for "CharSequence[] seqs = {" style tests. - There's been some informal discussion of packaging commonly used test utils as jtreg @library. This could be a good candidate. ArraysCharSequence is a candidate for testing utils lib. Basic impls that override no defaults are going to be increasingly important for testing. - I like your varargs assertsThrows. I have written a non-varargs one in test/.../Map/Defaults.java. Your varargs one of necessity violates the actual, expected, [message] pattern used by other TestNG assertions but I prefer it. This is also a candidate for utils lib. On May 1 2013, at 15:19 , Martin Buchholz wrote: Another version of this change is ready. No longer preliminary. Tests have been written. http://cr.openjdk.java.net/~martin/webrevs/openjdk8/getChars/

This kind of change is particularly subject to feature creep, and I am trying to restrain myself. I even addressed some of Ulf's suggestions. The "Msg" suffix is gone. I reverted changes to AbstractStringBuilder.replace. I kept the naming convention for getChars parameter names. Parameter names and exception details continue to be maddeningly unsystematic, but they should be a little better than before.

More information about the core-libs-dev mailing list