May 2015 - core-libs-dev

[9] RFR: 8080342: Incorrect GPL header causes RE script to miss swap to commercial header for licensee source bundle
by Naoto Sato 14 May '15

14 May '15

Hello, Another GPL header fix. Please review. http://cr.openjdk.java.net/~naoto/8080342/webrev.00/ Naoto

2 1

AbstractList etc. functionality as interfaces with default methods?
by Attila Szegedi 14 May '15

14 May '15

So I’m in a position where I’d need to have a class implement List, but it already extends something else, so I can’t have it extend AbstractList, which leaves me with a lot of boilerplate methods to implement. Would it seem like a good idea to reimagine AbstractList and friends as interfaces with default methods? Of course, I know that technically for backwards compatibility we can’t really do this, but what we could do is: public interface DefaultList<T> extends List<T> { … add all methods from AbstractList here as default methods … } public abstract class AbstractList<T> implements DefaultList<T> { } (I’ll hand-wave the issue of “protected int modCount” issue for now.) Actually, this seems like such an obvious idea that I’m 100% sure it must’ve been considered before, but I can’t find any related discussion. Attila.

9 17

8080330: (cs) Charset.availableCharsets failing with NPE on several platforms
by Alan Bateman 14 May '15

14 May '15

This method is currently broken in jdk9/dev and is causing several test failures. It looks like the changes JDK-8035302 missed a file. I need a Reviewer for the patch below. Additionally the test/sun/nio/cs/TestCompoundTest.java can be removed. I'm sure Sherman will have suggestions for other clean-up but for now I think it would be good to get jdk9/dev stable again. Thanks, Alan. diff --git a/make/data/charsetmapping/charsets b/make/data/charsetmapping/charsets --- a/make/data/charsetmapping/charsets +++ b/make/data/charsetmapping/charsets @@ -1833,10 +1833,3 @@ ## ######################################################## -charset x-COMPOUND_TEXT COMPOUND_TEXT - package sun.nio.cs.ext - type source - os unix - alias COMPOUND_TEXT # JDK historical - alias x11-compound_text - alias x-compound-text

2 1

Re: Updating existing JDK code to use InputStream.transferTo()
by Patrick Reinhart 14 May '15

14 May '15

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

1 0

RFR: 8071571: Move substring of same string to slow path
by Lev Priima 13 May '15

13 May '15

Please review small cleanup in java.lang.String: Issue: https://bugs.openjdk.java.net/browse/JDK-8071571 Webrev: http://cr.openjdk.java.net/~lpriima/8071571/0/webrev/ 46 tests from jdk9/dev/jdk/test/java/lang/String* passed locally. -- Lev

7 30

Re: <AWT Dev> Changes for JDK-8075327: moving jdk testlibraty files duplicated in hotspot to the common test repository
by Alexander Kulyakhtin 13 May '15

13 May '15

Chris, Following the feedback from Jon Gibbons, we have changed our current plan to the following: 1) We are waiting till the new jtreg is promoted. The new jtreg allows for specifying in the repository's TEST.properties file one or more paths to search for libraries 2) After the jtreg is out we are adding both path to jdk/testlibrary and path to toplevel/test/lib/share library to the jdk and hotspot TEST.properties files Under those testlibrary roots we are rearranging the directories to have the same jdk/test/lib structure (to provide for the jdk.test.lib package) 3) Same as with the current changes we are changing import package jdk.testlib.* to import package jdk.test.lib.* in the jdk and hotspot tests. This way, if the jtreg does not find a required class from the jdk.test.lib in the jdk/testlibrary it will look for it in the toplevel/test/lib/share test library. The same will be done for the hotspot tests. Best regards, Alexander ----- Original Message ----- From: chris.hegarty(a)oracle.com To: alexander.kulyakhtin(a)oracle.com Cc: core-libs-dev(a)openjdk.java.net, awt-dev(a)openjdk.java.net, stefan.sarne(a)oracle.com, yekaterina.kantserova(a)oracle.com, hotspot-dev(a)openjdk.java.net, jdk9-dev(a)openjdk.java.net Sent: Wednesday, May 13, 2015 7:48:06 PM GMT +03:00 Iraq Subject: Re: <AWT Dev> Changes fro JDK-8075327: moving jdk testlibraty files duplicated in hotspot to the common test repository Alexander, > On 13 May 2015, at 16:46, Alexander Kulyakhtin <alexander.kulyakhtin(a)oracle.com> wrote: > > Hi Chris, > > >> I suspect that these changes are best going directly into jdk9/dev, as >> opposed to a a downstream forest. > Yes, they are going directly to jdk9/dev, I forgot to add the group, adding now. > >> In may places '@library /lib/testlibrary ...' remains. Is this >> redundant, in many tests? If so, it be removed. > No, this is mostly not redundant, as the jdk/testlibrary comntains many files specific for jdk only. Only a relatively small number of files are duplicated between hotspot and jdk. Right, but mostly is not all. For example test/com/sun/net/httpserver/Test1.java should no longer include @library /lib/testlibrary. It depends only on SimpleSSLContext which is being moved. It is easy to see this by looking at the imports. Whatever script you have for updating these tests should also remove redundant values in the @library tag. I suspect there are a significant number of these ( a lot of tests just use one library class ). Just on that, is it for a library class to indicate that it depends on a class from another library? I believe this is the case for some library utilities. How can a test know this. It needs to be a library class that indicates this dependency.I see nothing in the webrev for this ( or maybe I missed it ). -Chris. > >> The changes are to the 'test' directory in the "top" repo? You are not >> proposing to add a new repo, right? > No, we are not proposing a new repo. The changes are in the 'top' repo. > >> test/lib/testlibrary/jdk/testlibrary/RandomFactory.java was updated >> recently in jdk9/dev. The version in your webrev is a little out of date. > We are going to upmerge the changes then > >> Is there any special update needed to jtreg to support this? > For thse changes no jtreg update is required. > However, there are plans for the jtreg to prohibit referencing any @libraries above the repo root unless such libraries are directly specified in TEST.properties > If jtreg thus prohibits our using /../../test/lib (because it's above the root) we will have to additionally make a change to jdk and hotspot TEST.properties > > Best regards, > Alexander

1 0

Re: <AWT Dev> Changes fro JDK-8075327: moving jdk testlibraty files duplicated in hotspot to the common test repository
by Alexander Kulyakhtin 13 May '15

13 May '15

Hi Chris, > I suspect that these changes are best going directly into jdk9/dev, as > opposed to a a downstream forest. Yes, they are going directly to jdk9/dev, I forgot to add the group, adding now. > In may places '@library /lib/testlibrary ...' remains. Is this > redundant, in many tests? If so, it be removed. No, this is mostly not redundant, as the jdk/testlibrary comntains many files specific for jdk only. Only a relatively small number of files are duplicated between hotspot and jdk. >The changes are to the 'test' directory in the "top" repo? You are not >proposing to add a new repo, right? No, we are not proposing a new repo. The changes are in the 'top' repo. >test/lib/testlibrary/jdk/testlibrary/RandomFactory.java was updated >recently in jdk9/dev. The version in your webrev is a little out of date. We are going to upmerge the changes then >Is there any special update needed to jtreg to support this? For thse changes no jtreg update is required. However, there are plans for the jtreg to prohibit referencing any @libraries above the repo root unless such libraries are directly specified in TEST.properties If jtreg thus prohibits our using /../../test/lib (because it's above the root) we will have to additionally make a change to jdk and hotspot TEST.properties Best regards, Alexander

2 1

Re: Changes fro JDK-8075327: moving jdk testlibraty files duplicated in hotspot to the common test repository
by Alexander Kulyakhtin 13 May '15

13 May '15

Adding jdk9-dev mailing list ----- Original Message ----- From: alexander.kulyakhtin(a)oracle.com To: core-libs-dev(a)openjdk.java.net, awt-dev(a)openjdk.java.net, hotspot-dev(a)openjdk.java.net Cc: stefan.sarne(a)oracle.com, yekaterina.kantserova(a)oracle.com, alan.bateman(a)oracle.com Sent: Wednesday, May 13, 2015 5:52:20 PM GMT +03:00 Iraq Subject: Changes fro JDK-8075327: moving jdk testlibraty files duplicated in hotspot to the common test repository Hi, Could you please, review the following tests-only changes to the hs-rt/jdk and hs-rt/test repositories. These changes are a part of the changes for "JDK-8075327: Merge jdk and hotspot test libraries" The changes are as follows: http://cr.openjdk.java.net/~akulyakh/8075327/jdk_patch/webrev/ http://cr.openjdk.java.net/~akulyakh/8075327/test_patch/webrev/ 1) Renaming jdk.testlibrary package to jdk.test.lib in the hs-rt/jdk repo, so it has the same name as the jdk.test.lib package in the hotspot repo. 2) Several files from the jdk/testlibrary have duplicates in the hotspot/testlibrary. We are moving those files from jdk/testlibrary to the upper-level hs-rt/test so they can be shared by jdk and hotspot (also updating @library directives to reflect that). If these proposed changes are acceptable then we'll merge the duplicates between the hs-rt/hotspot and hs-rt/test/lib files into hs-rt/test/lib and prepare a full review. Thank you very much for the review. Best regards, Alexander

1 0

Changes fro JDK-8075327: moving jdk testlibraty files duplicated in hotspot to the common test repository
by Alexander Kulyakhtin 13 May '15

13 May '15

Hi, Could you please, review the following tests-only changes to the hs-rt/jdk and hs-rt/test repositories. These changes are a part of the changes for "JDK-8075327: Merge jdk and hotspot test libraries" The changes are as follows: http://cr.openjdk.java.net/~akulyakh/8075327/jdk_patch/webrev/ http://cr.openjdk.java.net/~akulyakh/8075327/test_patch/webrev/ 1) Renaming jdk.testlibrary package to jdk.test.lib in the hs-rt/jdk repo, so it has the same name as the jdk.test.lib package in the hotspot repo. 2) Several files from the jdk/testlibrary have duplicates in the hotspot/testlibrary. We are moving those files from jdk/testlibrary to the upper-level hs-rt/test so they can be shared by jdk and hotspot (also updating @library directives to reflect that). If these proposed changes are acceptable then we'll merge the duplicates between the hs-rt/hotspot and hs-rt/test/lib files into hs-rt/test/lib and prepare a full review. Thank you very much for the review. Best regards, Alexander

2 1

RFR: 8079841: Buffer underflow with empty zip entry names
by Jeremy Manson 12 May '15

12 May '15

There's a fairly harmless buffer underflow with empty zip names in libzip. Martin has offered to sponsor this (right, Martin?). We detected this with ASan, an automatic memory error and leak checking tool built into Clang/LLVM. This is one of the reasons that Alexander Smundak asked if we could backport clang support to JDK8. Bug: https://bugs.openjdk.java.net/browse/JDK-8079841 Webrev: http://cr.openjdk.java.net/~jmanson/8079841/webrev.00/

3 2