Add getChars to CharSequence

[Peter Levart]

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.