Hi! How fast!
Tue, 15 Jun 2010 19:07:18 +0100 Andrew John Hughes <ahug...@redhat.com>: > 2010/6/15 Ivan Maidanski <iv...@mail.ru>: > > Hi! > > > > Here, the proposed changes of PrintStream are: > > 1. to be closer to the RI behavior (see changes for line_separator, > > out.flush(), append()); > > While some of these are sensible changes, how are you discovering the > 'RI behaviour' in the cases where it wouldn't be visible from simple > runtime testing? There are a lot of places in the Classpath saying that while the JDK docs say nothing (or, even, the opposite) about some feature, the RI behavior is followed. (For the samples, search for "RI" word in the Classpath source files.) Which one can't be visible, you think, from a test? > > > 2. some code optimizations (like removal of writeChars() pos/len > > parameters). > > > > ChangeLog entries: > > * java/io/PrintStream.java (line_separator): Convert static field to > > an instance one (to match the RI functionality). > > I don't see why you would want to make this an instance field. only to match the RI functionality ;) - test how BufferedWriter is working in the RI. > > > * java/io/PrintStream.java (error_occurred): Remove an unnecessary > > initialization to false. > > I think having it explicit does make things clearer. Ok. (Although, I don't think that explicitly setting it to false after it was implicitly set to the same value makes things clearer.) > > > * java/io/PrintStream.java (PrintStream): Use > > "new FileOutputStream(fileName)" instead of > > "new FileOutputStream(new File(fileName))". > > Ok (there are two instances of this, your ChangeLog only lists one) I know that. What's the preferred way of listing such things? specify with the arguments? I'll change the log. > > > * java/io/PrintStream.java (PrintStream): Throw NPE if out is null. > > Where is this specified? The Javadoc for this don't specify an NPE is thrown. > If OpenJDK does, then the documentation needs updating. We should include > a message saying what the problem was. the undocumented RI behavior - throw NPE for a null stream (instead of UnsupportedEncodingException one). > > > * java/io/PrintStream.java (writeChars, print, println): Don't pass > > "pos" and "len" parameters to writeChars() (as they are always set > > to 0 and buf.length, respectively). > > I think the original version should stay. It's clearer and avoids a > method call. Why is it clearer? (Is writeChars(char[] buf, int offset, int count) clearer than writeChars(char[] buf) (equipped with the appropriate doc)? And what's the method call is avoided? IMHO, it's arguable that it would be good to still keep for the private functions additional unnecessary arguments (and process them - e.g. call substring(offset, offset+count)). > > This change occurs twice but is listed here only once. Ok. I'll change. > > > * java/io/PrintStream.java (print): Call out.flush() only if needed > > (to match the RI). > > Where are these extra conditions coming from? Again, the RI behaves differently (the RI flushes the underlying stream only in case of '\n' while the Classpath flushes self (not the underlying stream directly) every time (provided auto_flush is on). > > > * java/io/PrintStream.java (lastIndexOfNewLine): New private static > > method (called from print() only). > > > * java/io/PrintStream.java (write): Remove unnecessary "&" operation. > > * java/io/PrintStream.java (print, write): Directly call out.flush() > > instead of flush() (the same as in the RI). > > * java/io/PrintStream.java (append): Call subSequence() also for > > "null" string (the same as in the RI). > > > > > > The append change looks good as do the flush() changes. I don't see > the reason for changing the ordering of the && statement as checking > auto_flush is the cheaper check. Ok. Reordering here is not a big deal for performance (IMHO, local var fetch should be faster than global memory access). > Additional arguments should stay. Still, I don't agree. See above. > > The ChangeLog needs some work as it was hard to follow and doesn't > match up to the patch. Why hard? due to changes grouping? And, please point me the places where it doesn't match. Regards. Thanks for reviewing the code (and for being the opponent). I'll resubmit the modified/final patch/changelog upon we agree the changes. > -- > Andrew :-) > > Free Java Software Engineer > Red Hat, Inc. (http://www.redhat.com)
