Add getChars to CharSequence (original) (raw)
Mike Duigou mike.duigou at oracle.com
Thu May 9 00:30:56 UTC 2013
- Previous message: Add getChars to CharSequence
- Next message: Add getChars to CharSequence
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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?
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.
- Previous message: Add getChars to CharSequence
- Next message: Add getChars to CharSequence
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]