Hi gents, Here are some comments regarding the updated webrev. >> >> (1) String(ByteBuffer, String) needs "@throw UEE". >> (2) It should be "default replacement byte array" not "replace string" >> for all >> getBytes() methods when malformed/unmappable >> (3) for decoding (new String) from ByteBuffer, since it is guaranteed >> that all >> bytes in the input ByteBuffer will be decoded, it might be desirable >> to clearly >> specify that the position of the buffer will be "advanced to its >> limit" ? >> (4) ln#1137 has an extra "*" >> (5) StringCoding.decode(cs, bb), why do you want to allocate a direct >> buffer >> here for "untrusted"? Basically the output buffer "ca" will always >> be a wrapper >> of a char[], all our charset implementation will have better >> performance if >> both input and output are "array based" (decode on array directly, >> instead of >> the "slow" ByteBuffer) >> > Overall I think this is looking quite good and I think Sherman has > captured the main issues. On #3 then wording such as ".. the position will > be updated" isn't clear enough to allow the method be tested, it needs to > make it clear that the position is advanced by the number of bytes that > were decoded.
Thanks for looking at this patch and agreeing to sponsor - I've pushed fixes for these issues at: http://cr.openjdk.java.net/~rwarburton/string-patch-webrev-10/ regards, Richard Warburton http://insightfullogic.com @RichardWarburto <http://twitter.com/richardwarburto>
