Re: BiCollector
Hello! In my StreamEx library I created a "pairing" collector which does similar job, but allows user to decide how to combine the results of two collectors. This adds more flexibility. The signature is like this: public static Collector pairing( Collector c1, Collector c2, BiFunction finisher) Having such collector, The proposed `toBoth(c1, c2)` can be implemented as simple as `pairing(c1, c2, Map::entry)`. OTOH if somebody wants to use their own Pair class, it would be `pairing(c1, c2, Pair::new)`. Sometimes you don't need a pair, but can create compound result object right here. E.g.: Collector summing = Collectors.reducing(BigDecimal.ZERO, BigDecimal::add); Collector averaging = pairing(summing, Collectors.counting(), (sum, count) -> sum.divide(BigDecimal.valueOf(count), RoundingMode.HALF_EVEN)); So locking to Map.Entry is entirely unnecessary. With best regards, Tagir Valeev. On Thu, Jun 14, 2018 at 6:11 AM Brian Goetz wrote: > I really like how BiCollector can be private, so all we'd have to expose > is toBoth(), and the arguments of toBoth() are just ordinary > collectors. So no new types or abstractions; just a multiplexing. > > The use of Map.Entry as a pair surrogate is unfortunate -- its > definitely not an Entry -- though I understand why you did this. I'm not > sure if this is fatal or not for a JDK method, but it's pretty bad*. > (You could generalize to n-ary, returning a List or array (you can take > a varargs of Collector), at the loss of sharp types for the results.) > > > *Yes, I'm sure one can find precedent of this being done; this has no > effect on whether it's bad. > > On 6/11/2018 8:39 AM, Peter Levart wrote: > > Hi, > > > > Have you ever wanted to perform a collection of the same Stream into > > two different targets using two Collectors? Say you wanted to collect > > Map.Entry elements into two parallel lists, each of them containing > > keys and values respectively. Or you wanted to collect elements into > > groups by some key, but also count them at the same time? Currently > > this is not possible to do with a single Stream. You have to create > > two identical streams, so you end up passing Supplier to other > > methods instead of bare Stream. > > > > I created a little utility Collector implementation that serves the > > purpose quite well: > > > > /** > > * A {@link Collector} implementation taking two delegate Collector(s) > > and producing result composed > > * of two results produced by delegating collectors, wrapped in {@link > > Map.Entry} object. > > * > > * @param the type of elements collected > > * @param the type of 1st delegate collector collected result > > * @param tye type of 2nd delegate collector collected result > > */ > > public class BiCollector implements Collector > Map.Entry, Map.Entry> { > > private final Collector keyCollector; > > private final Collector valCollector; > > > > @SuppressWarnings("unchecked") > > public BiCollector(Collector keyCollector, Collector > ?, V> valCollector) { > > this.keyCollector = (Collector) > > Objects.requireNonNull(keyCollector); > > this.valCollector = (Collector) > > Objects.requireNonNull(valCollector); > > } > > > > @Override > > public Supplier> supplier() { > > Supplier keySupplier = keyCollector.supplier(); > > Supplier valSupplier = valCollector.supplier(); > > return () -> new > > AbstractMap.SimpleImmutableEntry<>(keySupplier.get(), valSupplier.get()); > > } > > > > @Override > > public BiConsumer, T> accumulator() { > > BiConsumer keyAccumulator = > > keyCollector.accumulator(); > > BiConsumer valAccumulator = > > valCollector.accumulator(); > > return (accumulation, t) -> { > > keyAccumulator.accept(accumulation.getKey(), t); > > valAccumulator.accept(accumulation.getValue(), t); > > }; > > } > > > > @Override > > public BinaryOperator> combiner() { > > BinaryOperator keyCombiner = keyCollector.combiner(); > > BinaryOperator valCombiner = valCollector.combiner(); > > return (accumulation1, accumulation2) -> new > > AbstractMap.SimpleImmutableEntry<>( > > keyCombiner.apply(accumulation1.getKey(), > > accumulation2.getKey()), > > valCombiner.apply(accumulation1.getValue(), > > accumulation2.getValue()) > > ); > > } > > > > @Override > > public Function, Map.Entry> > > finisher() { > > Function keyFinisher = keyCollector.finisher(); > > Function valFinisher = valCollector.finisher(); > > return accumulation -> new AbstractMap.SimpleImmutableEntry<>( > > keyFinisher.apply(accumulation.getKey()), > > valFinisher.apply(accumulation.getValue()) > > ); > > } > > > > @Override > > public Set characteristics() { > > EnumSet intersection = > >
Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
On 13/06/2018 4:08 PM, joe darcy wrote: Hi David, On 5/24/2018 10:52 PM, David Holmes wrote: Here are the further minor updates so far in response to all the review comments. Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/ Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/ In Class.java, 3990 Class[] members = getNestMembers0(); 3991 // Can't actually enable this due to bootstrapping issues 3992 // assert(members.length != 1 || members[0] == this); // expected invariant from VM can these checks be enabled unconditionally without using the assert facility, throwing AssertionError or some other kind of error? Of course - but we don't want to pay the price of always checking something that would indicate an error on the VM side. There's an equivalent assertion on the VM side. Thanks, David Thanks, -Joe
Re: RFR(XS): 8202329 [AIX] Fix codepage mappings for IBM-943 and Big5
Hi Thomas, I've added testcase along with fix. Please find webrev at http://cr.openjdk.java.net/~aleonard/8202329/webrev.01/ Thanks, Bhaktavatsal Reddy -"core-libs-dev" wrote: - To: "Thomas Stüfe" From: "Bhaktavatsal R Maram" Sent by: "core-libs-dev" Date: 06/01/2018 01:34PM Cc: Java Core Libs Subject: Re: RFR(XS): 8202329 [AIX] Fix codepage mappings for IBM-943 and Big5 Hi Thomas, Sorry, I was on vacation. I will submit webrev with jtreg testcase. Thanks, Bhaktavatsal -"Thomas Stüfe" wrote: - To: Bhaktavatsal R Maram From: "Thomas Stüfe" Date: 05/23/2018 12:32AM Cc: Ichiroh Takiguchi , Volker Simonis , Java Core Libs Subject: Re: RFR(XS): 8202329 [AIX] Fix codepage mappings for IBM-943 and Big5 Hi Bhaktavatsal, the fix is fine. Reviewed. Thanks a lot for your work! Could you please (its okay in a future patch) add a regression test for IBM943 and IBM943C, in the form of a jtreg test? I would like to test conversions for both code pages (at least for the crucial tilde/backslash code points). Additionally, that when started on AIX with Ja_JP.IBM-943, we correctly default to IBM943C. As an example, here is an old test I wrote years ago. You won't be able to use it verbatim, so it is just a suggestion. Feel free to do it differently. http://cr.openjdk.java.net/~stuefe/other/IBM943MappingTest.java If you have not written jtreg tests before: http://openjdk.java.net/jtreg/ Also, under /test are many many examples. Best Regards, Thomas On Mon, May 21, 2018 at 9:47 AM, Bhaktavatsal R Maram wrote: > Hi Thomas, > > Is this fix ready to merge? > > Thanks, > Bhaktavatsal > > > > > -"Thomas Stüfe" wrote: - > To: Ichiroh Takiguchi , Bhaktavatsal R Maram > > From: "Thomas Stüfe" > Date: 05/11/2018 11:49AM > Cc: Volker Simonis , Java Core Libs > > Subject: Re: RFR(XS): 8202329 [AIX] Fix codepage mappings for IBM-943 and Big5 > > Hi, > > I'll test and review next week. We also have some in-house tests which > I'd like to run. > > You IBM folks should really apply for authorship so that this > contribution process gets streamlined. After all, if something breaks > in this code, you want to be able to fix it, yes? So even if you do > not contribute much else, more patches may be forthcoming. > > Of course I hope these are not your last contributions :) > > Best, Thomas > > > > On Fri, May 11, 2018 at 7:57 AM, Ichiroh Takiguchi > wrote: >> Hi. >> >> I tested this fix on AIX. >> >> I got following results. >> $ LANG=Ja_JP ~/jdk/bin/java PrintDefaultCharset >> Ja_JP x-IBM943C IBM-943CIBM-943C >> $ LANG=Ja_JP.IBM-943 ~/jdk/bin/java PrintDefaultCharset >> Ja_JP.IBM-943 x-IBM943C IBM-943CIBM-943C >> $ LANG=Zh_TW ~/jdk/bin/java PrintDefaultCharset >> Zh_TW x-IBM950IBM-950 IBM-950 >> $ LANG=Zh_TW.big5 ~/jdk/bin/java PrintDefaultCharset >> Zh_TW.big5 x-IBM950IBM-950 IBM-950 >> >> Also I reviewed source code, it's fine >> >> Since this testing requires locale installation for Ja_JP and Zh_TW, >> so it's not easy to test it... >> (At least, I think bos.loc.pc.Ja_JP and bos.loc.iso.Zh_TW filesets are >> required) >> >> >> On 2018-05-02 18:32, Volker Simonis wrote: >>> >>> Hi Bhaktavatsal Reddy, >>> >>> your change looks good. I can sponsor it. >>> >>> Just waiting for a second review... >>> >>> Thank you and best regards, >>> Volker >>> >>> >>> On Mon, Apr 30, 2018 at 11:29 AM, Bhaktavatsal R Maram >>> wrote: Hi All, Please review the fix. bug: https://bugs.openjdk.java.net/browse/JDK-8202329 webrev: http://cr.openjdk.java.net/~aleonard/8202329/webrev.00/ Thanks, Bhaktavatsal Reddy -"core-libs-dev" wrote: - To: Volker Simonis From: "Bhaktavatsal R Maram" Sent by: "core-libs-dev" Date: 04/26/2018 09:31PM Cc: Java Core Libs Subject: Re: [AIX] Fix codepage mappings in Java for IBM-943 and Big5 Hi Volker, Thank you. I will address your review comments and send webrev for review. - Bhaktavatsal Reddy -Volker Simonis wrote: - To: Bhaktavatsal R Maram From: Volker Simonis Date: 04/26/2018 09:12PM Cc: Java Core Libs Subject: Re: [AIX] Fix codepage mappings in Java for IBM-943 and Big5 Hi Bhaktavatsal Reddy, I've opened the following issue for this problem: 8202329: [AIX] Fix codepage mappings for IBM-943 and Big5 https://bugs.openjdk.java.net/browse/JDK-8202329 Looking at you fix, can you please replace the "#elif AIX" by "#ifdef AIX" and the original "#else" by "#ifdef __solaris__". The original else branch contains Solaris-only code anyway and it is an historical omission that there are still a lot of places in the code where "not Linux" implicitly means "Solaris", but that's often wrong. Regards, Volker On Thu, Apr 26, 2018
Re: RFR JDK-8066709 Make some JDK system properties read only
Thanks a lot for the explanation. Everything looks OK to me now. --Max > On Jun 13, 2018, at 10:10 PM, Roger Riggs wrote: > > Hi Joe, > > The CSR text is still in draft and got out of sync with the open review and > comments. > > The updated apiNote clarifies that changes made through the System.*property* > methods > may not affect the value cached during initialization. > The implementation changes affect a very short list of properties. > > The note on the methods is to raise awareness that individual properties may > have > different behavior and may be unpredictable. > > On 6/12/2018 10:10 PM, Weijun Wang wrote: >> In fact, why is setting user.name and user.home always evil? If I only want >> to set them on the command line so that a special "user environment" is >> used, why is it a problem? > There is no change to the ability to set properties on the command line. > The concern is with the predictability of (some) properties changing > dynamically while the application > is running. > > The property user.home is used for mime-types and mailcap files and user.name > is used in some > networking cases where a username is needed. >> >> In fact, we have a test setting user.home to an empty directory to avoid >> unexpected result because we cannot control the test runner's home directory. >> > Right, there is no problem with that > > Regards, Roger >> >> Thanks >> Max >> >> >>> On Jun 13, 2018, at 9:59 AM, Weijun Wang >>> wrote: >>> >>> Hi Roger >>> >>> 1. Should all occurrences of reading of these system properties be updated? >>> For example, the following one is not touched >>> >>> http://hg.openjdk.java.net/jdk/jdk/file/4d2e3f5abb48/src/java.base/share/classes/sun/security/tools/keytool/Main.java#l842 > That usage was in a tool and I did not modify any tools. >>> >>> 2. I assume that with this change not only there is no use calling >>> System.setProperty() in the application but also setting it on the command >>> line is now useless. Is this right? Do we need to make this clear in the >>> CSR? >>> > No change to command line setting of properties. > > Roger > >>> >>> Thanks >>> Max >>> >>> >>> On Jun 4, 2018, at 9:32 PM, Roger Riggs wrote: Please review a change to make the values of java.home, user.home, user.dir, and user.name effectively read-only for internal use. The values are cached during initialization and the cached values are used. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709/ Issue: https://bugs.openjdk.java.net/browse/JDK-8066709 CSR: https://bugs.openjdk.java.net/browse/JDK-8204235 Thanks, Roger >
Re: JDK 11 RFR of JDK-8205003: Replace selected link tags with linkplain in java.lang.Class
+1 -- Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Sent from my iPhone > On Jun 13, 2018, at 8:51 PM, joe darcy wrote: > > Hello, > > Please review the small patch below to address > > JDK-8205003: Replace selected link tags with linkplain in java.lang.Class > > Thanks, > > -Joe > > diff -r 0742a087710e src/java.base/share/classes/java/lang/Class.java > --- a/src/java.base/share/classes/java/lang/Class.javaWed Jun 13 13:12:50 > 2018 -0700 > +++ b/src/java.base/share/classes/java/lang/Class.javaWed Jun 13 17:50:12 > 2018 -0700 > @@ -820,7 +820,7 @@ > * primitive type or void, then the {@code Module} object for the > * {@code java.base} module is returned. > * > - * If this class is in an unnamed module then the {@link > + * If this class is in an unnamed module then the {@linkplain > * ClassLoader#getUnnamedModule() unnamed} {@code Module} of the class > * loader for this class is returned. > * > @@ -953,14 +953,14 @@ > * empty string if the class is in an unnamed package. > * > * If this class is a member class, then this method is equivalent to > - * invoking {@code getPackageName()} on the {@link #getEnclosingClass > + * invoking {@code getPackageName()} on the {@linkplain > #getEnclosingClass > * enclosing class}. > * > - * If this class is a {@link #isLocalClass local class} or an {@link > + * If this class is a {@linkplain #isLocalClass local class} or an > {@linkplain > * #isAnonymousClass() anonymous class}, then this method is equivalent > to > - * invoking {@code getPackageName()} on the {@link #getDeclaringClass > - * declaring class} of the {@link #getEnclosingMethod enclosing method} > or > - * {@link #getEnclosingConstructor enclosing constructor}. > + * invoking {@code getPackageName()} on the {@linkplain > #getDeclaringClass > + * declaring class} of the {@linkplain #getEnclosingMethod enclosing > method} or > + * {@linkplain #getEnclosingConstructor enclosing constructor}. > * > * If this class represents an array type then this method returns > the > * package name of the element type. If this class represents a primitive > @@ -2576,7 +2576,7 @@ > * @param name name of the desired resource > * @return A {@link java.io.InputStream} object; {@code null} if no > * resource with this name is found, the resource is in a > package > - * that is not {@link Module#isOpen(String, Module) open} to at > + * that is not {@linkplain Module#isOpen(String, Module) open} > to at > * least the caller module, or access to the resource is denied > * by the security manager. > * @throws NullPointerException If {@code name} is {@code null} > @@ -2675,7 +2675,7 @@ > * @return A {@link java.net.URL} object; {@code null} if no resource > with > * this name is found, the resource cannot be located by a URL, > the > * resource is in a package that is not > - * {@link Module#isOpen(String, Module) open} to at least the > caller > + * {@linkplain Module#isOpen(String, Module) open} to at least > the caller > * module, or access to the resource is denied by the security > * manager. > * @throws NullPointerException If {@code name} is {@code null} >
Re: JDK 11 RFR of JDK-8205003: Replace selected link tags with linkplain in java.lang.Class
+1 Mandy On 6/13/18 5:51 PM, joe darcy wrote: Hello, Please review the small patch below to address JDK-8205003: Replace selected link tags with linkplain in java.lang.Class Thanks, -Joe diff -r 0742a087710e src/java.base/share/classes/java/lang/Class.java --- a/src/java.base/share/classes/java/lang/Class.java Wed Jun 13 13:12:50 2018 -0700 +++ b/src/java.base/share/classes/java/lang/Class.java Wed Jun 13 17:50:12 2018 -0700 @@ -820,7 +820,7 @@ * primitive type or void, then the {@code Module} object for the * {@code java.base} module is returned. * - * If this class is in an unnamed module then the {@link + * If this class is in an unnamed module then the {@linkplain * ClassLoader#getUnnamedModule() unnamed} {@code Module} of the class * loader for this class is returned. * @@ -953,14 +953,14 @@ * empty string if the class is in an unnamed package. * * If this class is a member class, then this method is equivalent to - * invoking {@code getPackageName()} on the {@link #getEnclosingClass + * invoking {@code getPackageName()} on the {@linkplain #getEnclosingClass * enclosing class}. * - * If this class is a {@link #isLocalClass local class} or an {@link + * If this class is a {@linkplain #isLocalClass local class} or an {@linkplain * #isAnonymousClass() anonymous class}, then this method is equivalent to - * invoking {@code getPackageName()} on the {@link #getDeclaringClass - * declaring class} of the {@link #getEnclosingMethod enclosing method} or - * {@link #getEnclosingConstructor enclosing constructor}. + * invoking {@code getPackageName()} on the {@linkplain #getDeclaringClass + * declaring class} of the {@linkplain #getEnclosingMethod enclosing method} or + * {@linkplain #getEnclosingConstructor enclosing constructor}. * * If this class represents an array type then this method returns the * package name of the element type. If this class represents a primitive @@ -2576,7 +2576,7 @@ * @param name name of the desired resource * @return A {@link java.io.InputStream} object; {@code null} if no * resource with this name is found, the resource is in a package - * that is not {@link Module#isOpen(String, Module) open} to at + * that is not {@linkplain Module#isOpen(String, Module) open} to at * least the caller module, or access to the resource is denied * by the security manager. * @throws NullPointerException If {@code name} is {@code null} @@ -2675,7 +2675,7 @@ * @return A {@link java.net.URL} object; {@code null} if no resource with * this name is found, the resource cannot be located by a URL, the * resource is in a package that is not - * {@link Module#isOpen(String, Module) open} to at least the caller + * {@linkplain Module#isOpen(String, Module) open} to at least the caller * module, or access to the resource is denied by the security * manager. * @throws NullPointerException If {@code name} is {@code null}
Re: JDK 11 RFR of JDK-8205003: Replace selected link tags with linkplain in java.lang.Class
Hi Joe, +1 Brian On Jun 13, 2018, at 5:51 PM, joe darcy wrote: > Please review the small patch below to address > > JDK-8205003: Replace selected link tags with linkplain in java.lang.Class
JDK 11 RFR of JDK-8205003: Replace selected link tags with linkplain in java.lang.Class
Hello, Please review the small patch below to address JDK-8205003: Replace selected link tags with linkplain in java.lang.Class Thanks, -Joe diff -r 0742a087710e src/java.base/share/classes/java/lang/Class.java --- a/src/java.base/share/classes/java/lang/Class.java Wed Jun 13 13:12:50 2018 -0700 +++ b/src/java.base/share/classes/java/lang/Class.java Wed Jun 13 17:50:12 2018 -0700 @@ -820,7 +820,7 @@ * primitive type or void, then the {@code Module} object for the * {@code java.base} module is returned. * - * If this class is in an unnamed module then the {@link + * If this class is in an unnamed module then the {@linkplain * ClassLoader#getUnnamedModule() unnamed} {@code Module} of the class * loader for this class is returned. * @@ -953,14 +953,14 @@ * empty string if the class is in an unnamed package. * * If this class is a member class, then this method is equivalent to - * invoking {@code getPackageName()} on the {@link #getEnclosingClass + * invoking {@code getPackageName()} on the {@linkplain #getEnclosingClass * enclosing class}. * - * If this class is a {@link #isLocalClass local class} or an {@link + * If this class is a {@linkplain #isLocalClass local class} or an {@linkplain * #isAnonymousClass() anonymous class}, then this method is equivalent to - * invoking {@code getPackageName()} on the {@link #getDeclaringClass - * declaring class} of the {@link #getEnclosingMethod enclosing method} or - * {@link #getEnclosingConstructor enclosing constructor}. + * invoking {@code getPackageName()} on the {@linkplain #getDeclaringClass + * declaring class} of the {@linkplain #getEnclosingMethod enclosing method} or + * {@linkplain #getEnclosingConstructor enclosing constructor}. * * If this class represents an array type then this method returns the * package name of the element type. If this class represents a primitive @@ -2576,7 +2576,7 @@ * @param name name of the desired resource * @return A {@link java.io.InputStream} object; {@code null} if no * resource with this name is found, the resource is in a package - * that is not {@link Module#isOpen(String, Module) open} to at + * that is not {@linkplain Module#isOpen(String, Module) open} to at * least the caller module, or access to the resource is denied * by the security manager. * @throws NullPointerException If {@code name} is {@code null} @@ -2675,7 +2675,7 @@ * @return A {@link java.net.URL} object; {@code null} if no resource with * this name is found, the resource cannot be located by a URL, the * resource is in a package that is not - * {@link Module#isOpen(String, Module) open} to at least the caller + * {@linkplain Module#isOpen(String, Module) open} to at least the caller * module, or access to the resource is denied by the security * manager. * @throws NullPointerException If {@code name} is {@code null}
Re: BiCollector
I really like how BiCollector can be private, so all we'd have to expose is toBoth(), and the arguments of toBoth() are just ordinary collectors. So no new types or abstractions; just a multiplexing. The use of Map.Entry as a pair surrogate is unfortunate -- its definitely not an Entry -- though I understand why you did this. I'm not sure if this is fatal or not for a JDK method, but it's pretty bad*. (You could generalize to n-ary, returning a List or array (you can take a varargs of Collector), at the loss of sharp types for the results.) *Yes, I'm sure one can find precedent of this being done; this has no effect on whether it's bad. On 6/11/2018 8:39 AM, Peter Levart wrote: Hi, Have you ever wanted to perform a collection of the same Stream into two different targets using two Collectors? Say you wanted to collect Map.Entry elements into two parallel lists, each of them containing keys and values respectively. Or you wanted to collect elements into groups by some key, but also count them at the same time? Currently this is not possible to do with a single Stream. You have to create two identical streams, so you end up passing Supplier to other methods instead of bare Stream. I created a little utility Collector implementation that serves the purpose quite well: /** * A {@link Collector} implementation taking two delegate Collector(s) and producing result composed * of two results produced by delegating collectors, wrapped in {@link Map.Entry} object. * * @param the type of elements collected * @param the type of 1st delegate collector collected result * @param tye type of 2nd delegate collector collected result */ public class BiCollector implements CollectorMap.Entry, Map.Entry> { private final Collector keyCollector; private final Collector valCollector; @SuppressWarnings("unchecked") public BiCollector(Collector keyCollector, Collector?, V> valCollector) { this.keyCollector = (Collector) Objects.requireNonNull(keyCollector); this.valCollector = (Collector) Objects.requireNonNull(valCollector); } @Override public Supplier> supplier() { Supplier keySupplier = keyCollector.supplier(); Supplier valSupplier = valCollector.supplier(); return () -> new AbstractMap.SimpleImmutableEntry<>(keySupplier.get(), valSupplier.get()); } @Override public BiConsumer, T> accumulator() { BiConsumer keyAccumulator = keyCollector.accumulator(); BiConsumer valAccumulator = valCollector.accumulator(); return (accumulation, t) -> { keyAccumulator.accept(accumulation.getKey(), t); valAccumulator.accept(accumulation.getValue(), t); }; } @Override public BinaryOperator> combiner() { BinaryOperator keyCombiner = keyCollector.combiner(); BinaryOperator valCombiner = valCollector.combiner(); return (accumulation1, accumulation2) -> new AbstractMap.SimpleImmutableEntry<>( keyCombiner.apply(accumulation1.getKey(), accumulation2.getKey()), valCombiner.apply(accumulation1.getValue(), accumulation2.getValue()) ); } @Override public Function, Map.Entry> finisher() { Function keyFinisher = keyCollector.finisher(); Function valFinisher = valCollector.finisher(); return accumulation -> new AbstractMap.SimpleImmutableEntry<>( keyFinisher.apply(accumulation.getKey()), valFinisher.apply(accumulation.getValue()) ); } @Override public Set characteristics() { EnumSet intersection = EnumSet.copyOf(keyCollector.characteristics()); intersection.retainAll(valCollector.characteristics()); return intersection; } } Do you think this class is general enough to be part of standard Collectors repertoire? For example, accessed via factory method Collectors.toBoth(Collector coll1, Collector coll2), bi-collection could then be coded simply as: Map map = ... Map.Entry, List> keys_values = map.entrySet() .stream() .collect( toBoth( mapping(Map.Entry::getKey, toList()), mapping(Map.Entry::getValue, toList()) ) ); Map.Entry, Long> histogram_count = ThreadLocalRandom .current() .ints(100, 0, 10) .boxed() .collect( toBoth( groupingBy(Function.identity(), counting()), counting() ) ); Regards, Peter
Re: RFR JDK-8204930 Reader:nullReader() spec does not match the behavior
Hi Roger, Thanks for pointing this out: simpler and cleaner. Brian On Jun 13, 2018, at 2:06 PM, Roger Riggs wrote: > For the CSR, the easiest path to clarify the spec is to withdraw the CSR, > update the spec, > and add a note on the revised behavior. > > Then finalize the CSR again. That's enough to get it reviewed and approved. > > (Using a new CSR would just spread the behavior over two CSRs).
Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
Thanks for the explanation, Joe. I see the subtle difference on these terminologies. I'm okay with this patch. I like the other option better to remove @apiNote since the spec states that the duplicates may not be removed. Mandy On 6/12/18 5:49 PM, Joseph D. Darcy wrote: On 6/11/2018 10:38 PM, mandy chung wrote: On 6/11/18 10:16 PM, David Holmes wrote: Here is one further minor update from the CSR discussions: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v5-incr/src/java.base/share/classes/java/lang/Class.java.cdiff.html "This implementation" is fine, as used in many @implNote. Any reason why it has to be changed to "reference implementation"? To summarize the concern there, the phrase "This implementation..." when used elsewhere has a different meaning. Often the phrasing "This implementation..." or the more commonly used "The default implementation..." is used for text that is part of the contract of a method that can be overridden; that is, used to separate out the contract that is independent of which class or interface provides the implementation from the contract of a particular implementation. One example from an API I work on occurs for the method javax.lang.model.element.ElementVisitor.visitModule. The default method defined in an interface states "The default implementation visits a ModuleElement by calling visitUnknown..." and then various visitor classes define their own behavior for this method while still being able to @inheritDoc the general "visit a module element" contract of the visitModule method. However, java.lang.Class is final so for a particular JDK version there is only one implementation of the method in question. In that context "This implementation" doesn't mean "the implementation in this particular class or interface as opposed to the implementation in an a more specific subtype" it means "the implemetnation for the final method used in a particular JDK release." I think using the term "This implementation" in the latter context is misleading so I suggested the alternative wording David sent out for review. HTH, -Joe
Re: RFR JDK-8204930 Reader:nullReader() spec does not match the behavior
Hi Patrick, Yes, looks good to me too. For the CSR, the easiest path to clarify the spec is to withdraw the CSR, update the spec, and add a note on the revised behavior. Then finalize the CSR again. That's enough to get it reviewed and approved. (Using a new CSR would just spread the behavior over two CSRs). Thanks, Roger On 6/13/18 4:56 PM, Brian Burkhalter wrote: Hi Patrick, Not part of your change, but I noticed that at line 66 of Reader.java there is an extra parenthesis after ready(). In the test, the bug ID at line 39 could simply be appended to line 38. Otherwise looks good although I suppose given the specification update you’ll need an approved CSR before checking it in. Thanks, Brian On Jun 13, 2018, at 1:22 PM, Patrick Reinhart wrote: While looking into the current Reader Spec and the failing methods I found that the ready() method actually does behave wrong and I fixed this. For the case of mark() I would like to revise the specification to align with the Reader’s default behavior that states for the mark method: IOException - If the stream does not support mark(), or if some other I/O error occurs The new spec for those methods would then read like: 70 * The {@code markSupported()} method returns {@code false}. The 71 * {@code mark()} and {@code reset()} methods throw an {@code IOException}. Here the link to the webrev: http://cr.openjdk.java.net/~reinhapa/reviews/8204930/webrev
Re: RFR JDK-8204930 Reader:nullReader() spec does not match the behavior
Hi Patrick, Not part of your change, but I noticed that at line 66 of Reader.java there is an extra parenthesis after ready(). In the test, the bug ID at line 39 could simply be appended to line 38. Otherwise looks good although I suppose given the specification update you’ll need an approved CSR before checking it in. Thanks, Brian On Jun 13, 2018, at 1:22 PM, Patrick Reinhart wrote: > While looking into the current Reader Spec and the failing methods I found > that the ready() method actually does behave wrong and I fixed this. > > For the case of mark() I would like to revise the specification to align with > the Reader’s default behavior that states for the mark method: > > IOException - If the stream does not support mark(), or if some other I/O > error occurs > > The new spec for those methods would then read like: > > 70 * The {@code markSupported()} method returns {@code false}. The > 71 * {@code mark()} and {@code reset()} methods throw an {@code > IOException}. > > > Here the link to the webrev: > > http://cr.openjdk.java.net/~reinhapa/reviews/8204930/webrev
RFR JDK-8204930 Reader:nullReader() spec does not match the behavior
Hi everybody, While looking into the current Reader Spec and the failing methods I found that the ready() method actually does behave wrong and I fixed this. For the case of mark() I would like to revise the specification to align with the Reader’s default behavior that states for the mark method: IOException - If the stream does not support mark(), or if some other I/O error occurs The new spec for those methods would then read like: 70 * The {@code markSupported()} method returns {@code false}. The 71 * {@code mark()} and {@code reset()} methods throw an {@code IOException}. Here the link to the webrev: http://cr.openjdk.java.net/~reinhapa/reviews/8204930/webrev -Patrick
Re: RFR: JDK-8204172 Predicate::not should explicitly mention "NullPointerException - if target is null"
Hi Jim, Looks good to me. best regards, -- daniel On 13/06/18 20:36, Jim Laskey wrote: CSR: https://bugs.openjdk.java.net/browse/JDK-8204959 webrev: http://cr.openjdk.java.net/~jlaskey/8204172/webrev/index.html Thank you, — Jim
Re: RFR: JDK-8204973: Add build support for filtering translations
Hi Erik, The modifications to the logging test look good to me. Caveat: I don't speak chinese nor japanese ;-) cheers, -- daniel On 13/06/18 20:47, Erik Joelsson wrote: Hello, Oracle will reduce the number of languages that it maintains translations of JDK resources for. The current translations will remain in the source for now, but we need a way to filter out a set of translations at build time so that we only include the ones we support. This patch adds such a configuration option. It also changes how Oracle builds by using the option to exclude all translations except English, Japanese, Simplified Chinese and Traditional Chinese. Anyone else building OpenJDK will by default include all translations present in the source, just as before. I added a test that verifies this for builds with the "IMPLEMENTOR" field in the release file set to "Oracle Corporation". The test will not be run for other OpenJDK builds. I had to modify an existing test for java.logging which used various translations to verify localized log messages to only use translations that Oracle chooses to include. Since this is the second test that specifically verifies build behavior, I moved the previous such test together with this new test into a common top level test directory "build", under the jdk test root. I put these tests in the jdk tier3 test group. I have run all tier1, 2 and 3 tests in Mach 5 as well as specifically looked for tests that use the java.util.Locale class and ran them locally. Webrev: http://cr.openjdk.java.net/~erikj/8204973/webrev.01/index.html Bug: https://bugs.openjdk.java.net/browse/JDK-8204973 /Erik
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Thanks Roger! I pushed the changeset after s/unmappble/unmappable, it found three of them :-) Best, Joe On 6/12/18, 10:37 AM, Roger Riggs wrote: Hi Joe, Looks good to me. Typo: StringCoding.java:1026 "unmappble" (no new webrev needed) Regards, Roger On 6/12/2018 12:52 PM, Joe Wang wrote: Hi all, It's been a while since the 1st round of reviews. Thank you all for the valuable comments and suggestions! Note that Roger's last response went to nio-dev, but not core-libs-dev, I've therefore attached it below. CSR [2]: the CSR is now approved. Note the write method has been changed to writeString. Impl [3]: for performance reason and the different requirement from byte-string conversion for malformed/unmappable character handing, the implementation uses a specific method separate from the existing ones. Both Sherman and Alan preferred specific method than adding parameters to the APIs that might be error prone. Thanks Sherman for the code! JMH benchmark tests showed the new read method performs better than an operation that reads the bytes and convert to string. The gains start to be significant even at quite reasonable sizes (< 10K). [1] JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8202055 [3] webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ Please review. Thanks, Joe On 5/15/18, 7:51 AM, Roger Riggs wrote: Hi Joe, et. al. My $.02 on line separators: - We should avoid clever tricks trying to solve problems that are infrequent such as cross OS file line operations. Most files will be read/written on a single system so the line separators will be as expected. - Have/use APIs that split lines consistently accepting both line separators so developer code can be agnostic to line separators. aka BufferedReader.readLine for developers that are processing the contents *as lines*. Those other methods already exist; if there are any gaps in line oriented processing that's a separate task. - These new file methods are defined to handle Charset encoding/decoding and buffering. Since there are other methods to deal with files as lines these methods should not look for or break to lines. - Performance: adding code to look for line characters will slow it down and in most cases would have no impact since the line endings will match the system. - The strings passed to writeString (CharSequence) should have been constructed using the proper line separators. There are any number of methods that insert the os specific line terminator and its easy to write File.separator as needed. As for the method naming: I'd prefer "writeString" as a familiar term that isn't stretched too far by making the argument be a CharSequence. its fine to call a CharSequence a string (with a lower case s). $.02, Roger On 4/27/18 6:33 PM, Joe Wang wrote: On 4/27/2018 12:50 PM, fo...@univ-mlv.fr wrote: - Mail original - De: "Joe Wang" À: "Remi Forax" , "Alan Bateman" Cc: "nio-dev" , "core-libs-dev" Envoyé: Vendredi 27 Avril 2018 21:21:01 Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file Hi Rémi, Alan, Hi Joe, I'm not sure we'd want to replace system dependent line separators with '\n'. There are cases as you described where the replacement makes sense. However, there are other cases too. For example, the purpose is to read, edit a text file and then write it back. If this is on Windows, and if line separators are replaced with '\n', that would cause the resulting file not formatted properly in for example Notepad. There are cases where users write text files on Windows using Java, and only found the lines were not separated in Notepad. I agree that why the counterpart of readString() that write the string should inserts the platform's line separator. BTW, i just found that those methods are not named writeString, or writeCharSequence, i think they should. While readString() does not modify the original content (e.g. by replacing the platform's line separator with '\n'), write(String) won't either, by adding extra characters such as the line separator. I would think interfaces shall read along with the parameters. readString(Path) == read as a String from the specified Path (one could argue for readToString, readAsString, but we generally omit the preps) write(Path, CharSequence) == write the CharSequence to the file, since CharSequence is already in the method signature as a parameter, we probably don't want to add that to the name, otherwise it would read like repeating the word CharSequence. It is in a similar situation as write(Path, Iterable) where it was defined as writeLines(Path, Iterable). Files.write(Path, Iterable) is also specified to terminate each line with the platform's line separator. If readString does the replacement, it would be inconsistent. Anyway,
RFR: JDK-8204973: Add build support for filtering translations
Hello, Oracle will reduce the number of languages that it maintains translations of JDK resources for. The current translations will remain in the source for now, but we need a way to filter out a set of translations at build time so that we only include the ones we support. This patch adds such a configuration option. It also changes how Oracle builds by using the option to exclude all translations except English, Japanese, Simplified Chinese and Traditional Chinese. Anyone else building OpenJDK will by default include all translations present in the source, just as before. I added a test that verifies this for builds with the "IMPLEMENTOR" field in the release file set to "Oracle Corporation". The test will not be run for other OpenJDK builds. I had to modify an existing test for java.logging which used various translations to verify localized log messages to only use translations that Oracle chooses to include. Since this is the second test that specifically verifies build behavior, I moved the previous such test together with this new test into a common top level test directory "build", under the jdk test root. I put these tests in the jdk tier3 test group. I have run all tier1, 2 and 3 tests in Mach 5 as well as specifically looked for tests that use the java.util.Locale class and ran them locally. Webrev: http://cr.openjdk.java.net/~erikj/8204973/webrev.01/index.html Bug: https://bugs.openjdk.java.net/browse/JDK-8204973 /Erik
RFR: JDK-8204172 Predicate::not should explicitly mention "NullPointerException - if target is null"
CSR: https://bugs.openjdk.java.net/browse/JDK-8204959 webrev: http://cr.openjdk.java.net/~jlaskey/8204172/webrev/index.html Thank you, — Jim
Re: RFR(xs): 8204663: clean up remaining native parts after JDK-8187631
Thank you Paul! On Wed, Jun 13, 2018 at 6:19 PM, Paul Sandoz wrote: > +1 > Paul. > >> On Jun 13, 2018, at 2:56 AM, Thomas Stüfe wrote: >> >> May I have a second review please. >> >> Thanks, Thomas >> >> On Mon, Jun 11, 2018, 15:13 Thomas Stüfe wrote: >> >>> Hi, >>> >>> may I please have reviews for this small cleanup. >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8204663 >>> Webrev: >>> http://cr.openjdk.java.net/~stuefe/webrevs/JDK-8204663-clean-up-after-JDK-8187631/webrev.00/webrev/ >>> >>> This change removes some native functions from FOS/FIS which are >>> orphaned and unused since "JDK-8187631: Refactor FileDescriptor close >>> implementation". >>> >>> Tests on jdk-submit came back clean save one strange test error on >>> windows which I cannot reproduce locally. I am investigating that one. >>> >>> Thank you, Thomas >>> >
Re: RFR: 8204967: Resolve disabled warnings for libunpack
Does -Wimplicit-fallthrough=3 work for gnu c and clang? unpack.cpp redundant comment 3713 // else fall through: +1 — Jim > On Jun 13, 2018, at 1:57 PM, Srinivas Dama wrote: > > Hi, > > Please review http://cr.openjdk.java.net/~sdama/8204967/webrev.00/ > for https://bugs.openjdk.java.net/browse/JDK-8204967 > > Regards, > Srinivas
Re: RFR: [11] 8202537: CLDR 33
Hi Rachna, A couple of comments: - NumberingSystemsParseHandler.java Since the code substitutes latin digits for supplementary digits, it can skip line 68-79. - A test should be written for the above substitution. Naoto On 6/12/18 10:33 PM, Rachna Goel wrote: Hi, Kindly review fix for JDK-8202537. Fix is to upgrade Unicode consortium's CLDR data into JDK from current version 29 to 33. For more info : http://cldr.unicode.org/index/downloads/cldr-33 Bug : https://bugs.openjdk.java.net/browse/JDK-8202537 Patch: http://cr.openjdk.java.net/~rgoel/JDK-8202537/webrev.06/index.html
RFR: 8204967: Resolve disabled warnings for libunpack
Hi, Please review http://cr.openjdk.java.net/~sdama/8204967/webrev.00/ for https://bugs.openjdk.java.net/browse/JDK-8204967 Regards, Srinivas
Re: [JDK 11] RFR 8204944: Remove java/util/Map/InPlaceOpsCollisions.java from ProblemList.txt
+1 Paul. > On Jun 13, 2018, at 2:31 AM, Amy Lu wrote: > > Please review this patch to remove java/util/Map/InPlaceOpsCollisions.java > from ProblemList.txt, > related bug JDK-8203387 (JDK-8203425) has been fixed. > > Thanks, > Amy > > --- old/test/jdk/ProblemList.txt2018-06-13 17:23:33.0 +0800 > +++ new/test/jdk/ProblemList.txt2018-06-13 17:23:32.0 +0800 > @@ -853,8 +853,6 @@ > > # jdk_util > > -java/util/Map/InPlaceOpsCollisions.java 8203387 generic-all > - > > > # jdk_instrument > >
Re: RFR(xs): 8204663: clean up remaining native parts after JDK-8187631
+1 Paul. > On Jun 13, 2018, at 2:56 AM, Thomas Stüfe wrote: > > May I have a second review please. > > Thanks, Thomas > > On Mon, Jun 11, 2018, 15:13 Thomas Stüfe wrote: > >> Hi, >> >> may I please have reviews for this small cleanup. >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8204663 >> Webrev: >> http://cr.openjdk.java.net/~stuefe/webrevs/JDK-8204663-clean-up-after-JDK-8187631/webrev.00/webrev/ >> >> This change removes some native functions from FOS/FIS which are >> orphaned and unused since "JDK-8187631: Refactor FileDescriptor close >> implementation". >> >> Tests on jdk-submit came back clean save one strange test error on >> windows which I cannot reproduce locally. I am investigating that one. >> >> Thank you, Thomas >>
Re: RFR: [11] 8202537: CLDR 33
Hi Roger, No update is done mechanically between CLDR data and .xml files. CLDR's data in .xml files are generated into resourcebundles at build time by cldrconverter tool. Already existing regression test "test/jdk/sun/text/resources/LocaleDataTest.java" verify that same data is retrieved by APIs. Thanks, Rachna* * On 6/13/18 8:06 PM, Roger Riggs wrote: Hi Rachna, How much of the updates are done mechanically between the CLDR data and the .xml files? Manually reviewing that many files is likely to be error prone. Thanks, Roger On 6/13/2018 1:33 AM, Rachna Goel wrote: Hi, Kindly review fix for JDK-8202537. Fix is to upgrade Unicode consortium's CLDR data into JDK from current version 29 to 33. For more info : http://cldr.unicode.org/index/downloads/cldr-33 Bug : https://bugs.openjdk.java.net/browse/JDK-8202537 Patch: http://cr.openjdk.java.net/~rgoel/JDK-8202537/webrev.06/index.html -- Thanks, Rachna
Re: RFR: [11] 8202537: CLDR 33
Hi Rachna, How much of the updates are done mechanically between the CLDR data and the .xml files? Manually reviewing that many files is likely to be error prone. Thanks, Roger On 6/13/2018 1:33 AM, Rachna Goel wrote: Hi, Kindly review fix for JDK-8202537. Fix is to upgrade Unicode consortium's CLDR data into JDK from current version 29 to 33. For more info : http://cldr.unicode.org/index/downloads/cldr-33 Bug : https://bugs.openjdk.java.net/browse/JDK-8202537 Patch: http://cr.openjdk.java.net/~rgoel/JDK-8202537/webrev.06/index.html
Re: RFR JDK-8066709 Make some JDK system properties read only
Hi Joe, The CSR text is still in draft and got out of sync with the open review and comments. The updated apiNote clarifies that changes made through the System.*property* methods may not affect the value cached during initialization. The implementation changes affect a very short list of properties. The note on the methods is to raise awareness that individual properties may have different behavior and may be unpredictable. On 6/12/2018 10:10 PM, Weijun Wang wrote: In fact, why is setting user.name and user.home always evil? If I only want to set them on the command line so that a special "user environment" is used, why is it a problem? There is no change to the ability to set properties on the command line. The concern is with the predictability of (some) properties changing dynamically while the application is running. The property user.home is used for mime-types and mailcap files and user.name is used in some networking cases where a username is needed. In fact, we have a test setting user.home to an empty directory to avoid unexpected result because we cannot control the test runner's home directory. Right, there is no problem with that Regards, Roger Thanks Max On Jun 13, 2018, at 9:59 AM, Weijun Wang wrote: Hi Roger 1. Should all occurrences of reading of these system properties be updated? For example, the following one is not touched http://hg.openjdk.java.net/jdk/jdk/file/4d2e3f5abb48/src/java.base/share/classes/sun/security/tools/keytool/Main.java#l842 That usage was in a tool and I did not modify any tools. 2. I assume that with this change not only there is no use calling System.setProperty() in the application but also setting it on the command line is now useless. Is this right? Do we need to make this clear in the CSR? No change to command line setting of properties. Roger Thanks Max On Jun 4, 2018, at 9:32 PM, Roger Riggs wrote: Please review a change to make the values of java.home, user.home, user.dir, and user.name effectively read-only for internal use. The values are cached during initialization and the cached values are used. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709/ Issue: https://bugs.openjdk.java.net/browse/JDK-8066709 CSR: https://bugs.openjdk.java.net/browse/JDK-8204235 Thanks, Roger
Re: RFR(xs): 8204663: clean up remaining native parts after JDK-8187631
May I have a second review please. Thanks, Thomas On Mon, Jun 11, 2018, 15:13 Thomas Stüfe wrote: > Hi, > > may I please have reviews for this small cleanup. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8204663 > Webrev: > http://cr.openjdk.java.net/~stuefe/webrevs/JDK-8204663-clean-up-after-JDK-8187631/webrev.00/webrev/ > > This change removes some native functions from FOS/FIS which are > orphaned and unused since "JDK-8187631: Refactor FileDescriptor close > implementation". > > Tests on jdk-submit came back clean save one strange test error on > windows which I cannot reproduce locally. I am investigating that one. > > Thank you, Thomas >
[JDK 11] RFR 8204944: Remove java/util/Map/InPlaceOpsCollisions.java from ProblemList.txt
Please review this patch to remove java/util/Map/InPlaceOpsCollisions.java from ProblemList.txt, related bug JDK-8203387 (JDK-8203425) has been fixed. Thanks, Amy --- old/test/jdk/ProblemList.txt 2018-06-13 17:23:33.0 +0800 +++ new/test/jdk/ProblemList.txt 2018-06-13 17:23:32.0 +0800 @@ -853,8 +853,6 @@ # jdk_util -java/util/Map/InPlaceOpsCollisions.java 8203387 generic-all - # jdk_instrument
Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
Hi David, On 5/24/2018 10:52 PM, David Holmes wrote: Here are the further minor updates so far in response to all the review comments. Incremental corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3-incr/ Full corelibs webrev: http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v3/ In Class.java, 3990 Class[] members = getNestMembers0(); 3991 // Can't actually enable this due to bootstrapping issues 3992 // assert(members.length != 1 || members[0] == this); // expected invariant from VM can these checks be enabled unconditionally without using the assert facility, throwing AssertionError or some other kind of error? Thanks, -Joe