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.
