Hi Pavel,
On 14.05.2015 01:28, Pavel Rappo wrote:
Let me start then.
1. I've seen several cases where current behaviour
while ((n = in.read(buffer)) > 0)
~~~
has been changed to
while ((read = this.read(buffer, 0, TRANSFER_BUFFER_SIZE)) >= 0)
~~~
Those things jump out at you, but don't seem to be harmful, since the only case
where:
java.io.InputStream.read(byte[], int off, int len)
java.io.InputStream#read(byte[])
may return 0, is when len == 0. And it's never the case here. Unless, of course,
some misbehaving implementation of InputStream is used.
Do you think we should change our general behaviour to "> -1" instead of ">=0"
?
(Currently being discussed on core-libs-dev)
I saw that...
2.
jdk/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/utils/JavaUtils.java
Some of the refactored methods there seem to be unused:
getBytesFromFile(String)
writeBytesToFilename(String, byte[])
Should we remove them instead?
When it's no longer used, it makes no sense to have this method I would say.
Due to the edit process has took
place within a simple editor, I simply overlooked that fact.
No problem. In fact I will need to check this with my colleagues to make sure we
can safely do this. Otherwise if this thing, say, is regularly updated from some
Apache project, they might bring it (and/or dependencies on it) the next time
they update. This doesn't sound right to me. Thus we have to be super careful
even with the update you've proposed.
Or we leave that out in those special cases where we use "external" sources
4. I wonder if javax.management.loading.MLet.loadLibraryAsResource and
sun.net.www.MimeLauncher.run could be refactored with more appropriate
Files.copy(is, file.toPath())
as it seems to fit there perfectly.
I will try to do so and send in a new Patch for this...
It's absolutely fine for now. You can leave it as it is. java.nio.Files' time
will come.
I've put this on my list of things to do later then... ;-)
6. I've run into several cases where code use explicit `flush()` for
ByteArrayOutputStream, BufferedOutputStream. For the former one it's never
needed. The latter one does it automatically on `close()`.
Should we still call them?
There I'm not really sure, as a matter of fact, the close() is being done on
the try block exit and that is
being done after putting the return value on the stack as I believe (but I
could be wrong).
For the reason of my uncertainty I left the flush() method call in...
Which one is that you're not sure about?
BTW, looks like we've missed one more snippet in sun.print.UnixPrintJob:
try {
while ((cread = br.read(buffer, 0, buffer.length)) >=0) {
bw.write(buffer, 0, cread);
}
br.close();
bw.flush();
bw.close();
} catch (IOException e) {
notifyEvent(PrintJobEvent.JOB_FAILED);
throw new PrintException (e);
}
Sould I make the change or will you do it?
7. org.jcp.xml.dsig.internal.dom.Utils.readBytesFromStream is a little bit
awkward since it used 1024 as some sort of a cut-off.
It might've had something to do with EOF detection, but I'm not sure.
The way I was coming to the current implementation was based on due the fact,
that we almost would have one read
operation iteration more in case the last read amount would be less than 1024
but instead having a comparison more
in each loop.
The same thing as with (2). I don't like the package name. It might be synced
from the outside of the JDK.
Seem reasonable to me too.
It seems ok to remove the FileNotFoundException branch. Or I'm missing
something.
No, I thing you're right due to the fact that the FileNotFoundException extends
IOException anyway. Or would a
multi catch block make more sense?
} catch (FileNotFoundException | IOException e) {
notifyEvent(PrintJobEvent.JOB_FAILED);
throw new PrintException(e.toString());
}
I think we'd better remove it. As we both noticed, FileNotFoundException is a
subtype of IOException.
That's totally fine for me
When looking closer to that code part now, I ask myself if it's a good thing to
have the causing stack being lost
there and would it not be better to pass the causing exception?
} catch (FileNotFoundException | IOException e) {
notifyEvent(PrintJobEvent.JOB_FAILED);
throw new PrintException(e);
}
It would, you're right. Unfortunately we don't know how this exception might be
used. Maybe there was a reason for not including the cause or it might have been
just done before the "cause" field in Throwable became available (before
JDK1.4)
I'm pretty sure about this. Here I guess we could ask the responsible
group. Besides that, we could compose
the PrintException in a way, that the message stays the same and we
additionally have the causing exception:
} catch (IOException e) {
notifyEvent(PrintJobEvent.JOB_FAILED);
throw new PrintException(e.toString(), e);
}
9. I don't think I would agree with some changes in sun.net.www.MimeLauncher.run
as they obviously make the new version not equivalent to the old one. Non
swallowed IOException, additional null check. Way too much for the cleanup
change.
Well, the null check I agree. To have the same behaviour, we simply skip remove
the null check.
Your objection about the not swallowed IOException I can not see because it's
being swallowed in the
outer try-catch block at last. So I think that will result in the same result
here.
Yes, it's swallowed by the outer block. But as far as I can see it now doesn't
do all these things after an exception is thrown from inside `transferTo`:
int inx = 0;
String c = execPath;
while ((inx = c.indexOf("%t")) >= 0) {
c = c.substring(0, inx) + uc.getContentType()
+ c.substring(inx + 2);
}
boolean substituted = false;
while ((inx = c.indexOf("%s")) >= 0) {
c = c.substring(0, inx) + ofn + c.substring(inx + 2);
substituted = true;
}
if (!substituted)
c = c + " <" + ofn;
// System.out.println("Execing " +c);
Runtime.getRuntime().exec(c);
Now I got the your point. I should stop writing emails and looking into
code at 1 am ;-)
-------------------------------------------------------------------------------
P.S. And yes, Remi is right, we'd better split this thing into several pieces --
according to areas.
I agree, the Linux find command does not stop on responsible groups
boundaries ;-)
Following 3-4 weeks I might become unresponsive on core-libs or privately as I'm
busy with another project (just in case).
No problem, thanks for informing me. I also applied for author role, so
that it's becoming easier, to maintain webrev's
by myself instead of posting diff's and you have to create again a
webrev in turn....
Thanks a lot for doing this, Patrick. It's really useful.
You're welcome, I'de love to some more in the future...
-Patrick
