Il giorno mar, 15/06/2010 alle 22.59 +0400, Ivan Maidanski ha scritto: > 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?
Hi Ivan, We can't look at the OpenJDK code and then easily make the Classpath one match unless where obvious fixes are in place (if RI is the closed one, we can *never* make this even of obvious fixes), can you please provide us the tests you wrote to detect the faulty functionality? > > > 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. I can't see why you want this change. This field is private, and I think its value never changes during the lifetime of the VM, Actually, even in the JDK the path separator is a field in the File class that is cached at class initialisation time, and finally, the field is static anyway. I can't really see how this impacts the functionality, can you please provide a test? > > > > > * 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.) I don't completely share Andrew's point of view on this one, and although in this specific case the line of code is harmless, I believe that we should always trust the VM to initialise class and instance fields for us to the default values (which is mandatory anyway), so your change is OK for me. > > > > > * 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. Not sure about this, I guess we use a couple of different ways, I would add in parenthesis the arguments of the two constructors. > > > * 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). I would add a comment so (and, again, a test in Mauve). > > > > > * 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). Can you provide some minimal functional test to prove that different behaviour affects user code (sometime those things do affect functionality, but at times not)? > > 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). Well, this depends... Cheers, Mario -- pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF Proud GNU Classpath developer: http://www.classpath.org/ Read About us at: http://planet.classpath.org OpenJDK: http://openjdk.java.net/projects/caciocavallo/ Please, support open standards: http://endsoftpatents.org/
