Integrated: 8287440: Typo in package-info.java of java.util.random

[Anthony Vanelverdinghe]

On Wed, 18 May 2022 11:45:46 GMT, Anthony Vanelverdinghe 
 wrote:

> The change is trivial, but I'll need help from someone to create a JBS issue 
> & sponsor this PR.

This pull request has now been integrated.

Changeset: 3d2d0395
Author:    Anthony Vanelverdinghe 
Committer: Jaikiran Pai 
URL:   
https://git.openjdk.java.net/jdk/commit/3d2d039538b906cedd9188ed94b7ba55c275ff7f
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod

8287440: Typo in package-info.java of java.util.random

Reviewed-by: darcy, iris, jpai

-

PR: https://git.openjdk.java.net/jdk/pull/8766

Re: RFR: 8287440: Typo in package-info.java of java.util.random

[Anthony Vanelverdinghe]

On Sat, 28 May 2022 10:00:50 GMT, Jaikiran Pai  wrote:

>> The change is trivial, but I'll need help from someone to create a JBS issue 
>> & sponsor this PR.
>
> @anthonyvdotbe, can you also please change the copyright year on that file 
> from `Copyright (c) 2021,` to `Copyright (c) 2021, 2022,`

Thanks for the heads-up @jaikiran I've fixed the copyright year

-

PR: https://git.openjdk.java.net/jdk/pull/8766

Re: RFR: 8287440: Typo in package-info.java of java.util.random [v2]

[Anthony Vanelverdinghe]

> The change is trivial, but I'll need help from someone to create a JBS issue 
> & sponsor this PR.

Anthony Vanelverdinghe has updated the pull request incrementally with one 
additional commit since the last revision:

  Correct copyright year
  
  Signed-off-by: Anthony Vanelverdinghe 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8766/files
  - new: https://git.openjdk.java.net/jdk/pull/8766/files/1ed395f3..aeb62f1b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8766=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8766=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8766.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8766/head:pull/8766

PR: https://git.openjdk.java.net/jdk/pull/8766

RFR: 8287440: Typo in package-info.java of java.util.random

[Anthony Vanelverdinghe]

The change is trivial, but I'll need help from someone to create a JBS issue & 
sponsor this PR.

-

Commit messages:
 - Copy edit: remove pleonasm

Changes: https://git.openjdk.java.net/jdk/pull/8766/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8766=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8287440
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8766.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8766/head:pull/8766

PR: https://git.openjdk.java.net/jdk/pull/8766

Re: RFR: 8279031: Add API note to ToolProvider about being reusable/reentrant [v2]

[Anthony Vanelverdinghe]

On Tue, 24 May 2022 04:50:58 GMT, Jaikiran Pai  wrote:

>> Christian Stein has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Update src/java.base/share/classes/java/util/spi/ToolProvider.java
>>   
>>   Co-authored-by: Anthony Vanelverdinghe 
>
> src/java.base/share/classes/java/util/spi/ToolProvider.java line 59:
> 
>> 57:  * a tool may be the target of multiple {@code run} method invocations,
>> 58:  * and reentrant means that multiple invocations of {@code run} may occur
>> 59:  * concurrently.
> 
> Hello @sormuras,
> 
>> reentrant means that multiple invocations of {@code run} may occur
>> concurrently.
> 
> My understanding of re-entrant was that the same method could be re-invoked 
> on the same thread while the current method execution is in progress (a 
> recursion). Whereas "multiple invocations may occur concurrently" sounds more 
> like multiple threads invoking this concurrently (i.e. thread-safety). Which 
> of these 2 characteristics are we recommending here?

I agree with @jaikiran that reentrant is much more common in the sense of "on 
the same thread". Plus that the JDK itself uses "ReentrantLock" in this 
meaning. The JDK uses either "thread-safe" or "synchronized" for the "multiple 
threads" meaning.

Actually, being thread-safe implies being reusable, so I'd just phrase it as:

> It is recommended that tools implementing this interface are either
> thread-safe, or clearly document any limitations and restrictions.

Also: should this be an `@implNote`, rather than an `@apiNote`, since it's 
about "tools implementing this interface"?

-

PR: https://git.openjdk.java.net/jdk/pull/8833

Re: RFR: 8279031: Add API note to ToolProvider about being reusable/reentrant [v2]

[Anthony Vanelverdinghe]

On Mon, 23 May 2022 10:34:51 GMT, Christian Stein  wrote:

>> This commit adds an  API note to ToolProvider about being reusable/reentrant.
>
> Christian Stein has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update src/java.base/share/classes/java/util/spi/ToolProvider.java
>   
>   Co-authored-by: Anthony Vanelverdinghe 

src/java.base/share/classes/java/util/spi/ToolProvider.java line 57:

> 55:  * reusable and reentrant, or should clearly document any limitations and
> 56:  * restrictions. In this context, reusable means that any one instance of
> 57:  * a tool may be a the target of multiple {@code run} method invocations,

Suggestion:

 * a tool may be the target of multiple {@code run} method invocations,

-

PR: https://git.openjdk.java.net/jdk/pull/8833

Re: RFR: JDK-8277095 : Empty streams create too many objects

[Anthony Vanelverdinghe]

On Sun, 7 Nov 2021 07:51:06 GMT, Michael Bien  wrote:

>>> wouldn't this make streams no longer lazy if the collection is empty?
>>> 
>>> ```java
>>> List list = new ArrayList<>();
>>> Stream stream = list.stream();
>>> 
>>> list.addAll(List.of("one", "two", "three"));
>>> 
>>> stream.forEach(System.out::println); // prints one two three
>>> ```
>> 
>> I did not consider this case, thank you for bringing it up. I have always 
>> found this behaviour a bit strange and have never used it "in the real 
>> world". It is also not consistent between collections. Here is an example 
>> with four collections: ArrayList, CopyOnWriteArrayList, 
>> ConcurrentSkipListSet and ArrayBlockingQueue:
>> 
>> 
>> import java.util.ArrayList;
>> import java.util.Arrays;
>> import java.util.Collection;
>> import java.util.List;
>> import java.util.Objects;
>> import java.util.concurrent.ArrayBlockingQueue;
>> import java.util.concurrent.ConcurrentSkipListSet;
>> import java.util.concurrent.CopyOnWriteArrayList;
>> import java.util.function.Supplier;
>> import java.util.stream.IntStream;
>> 
>> public class LazyStreamDemo {
>> public static void main(String... args) {
>> List>> suppliers =
>> List.of(ArrayList::new, // fast-fail
>> CopyOnWriteArrayList::new, // snapshot
>> ConcurrentSkipListSet::new, // weakly-consistent
>> () -> new ArrayBlockingQueue<>(10) // 
>> weakly-consistent
>> );
>> for (Supplier> supplier : suppliers) {
>> Collection c = supplier.get();
>> System.out.println(c.getClass());
>> IntStream stream = c.stream()
>> .sorted()
>> .filter(Objects::nonNull)
>> .mapToInt(String::length)
>> .sorted();
>> 
>> c.addAll(List.of("one", "two", "three", "four", "five"));
>> System.out.println("stream = " + 
>> Arrays.toString(stream.toArray()));
>> }
>> }
>> }
>> 
>> 
>> The output is:
>> 
>> 
>> class java.util.ArrayList
>> stream = [3, 3, 4, 4, 5]
>> class java.util.concurrent.CopyOnWriteArrayList
>> stream = []
>> class java.util.concurrent.ConcurrentSkipListSet
>> stream = []
>> class java.util.concurrent.ArrayBlockingQueue
>> stream = [3, 3, 4, 4, 5]
>> 
>> 
>> At least with the EmptyStream we would have consistent output of always []
>
> @kabutz I agree that i wouldn't consider it clean code to use a stream like i 
> put into the example. I only brought it up because it might break existing 
> code, since i think streams are expected to be lazy. Interesting to see that 
> they are in fact not lazy in all situations - i put that into my notes.

Edit: actually I think the implementation of `Collection::stream` could simply 
be changed to:


default Stream stream() {
var spliterator = spliterator();
if(spliterator.hasCharacteristics(Spliterator.IMMUTABLE | 
Spliterator.CONCURRENT) && isEmpty()) {
return Stream.empty();
}
return StreamSupport.stream(spliterator, false);
}


Note that the spliterators of methods such as `Collections::emptyList`, 
`List::of`, `List::copyOf`, `Set::of`, ... currently don't have the `IMMUTABLE` 
characteristic though, so they should be updated.

---

The Javadoc for 
[java.util.stream](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/stream/package-summary.html)
 is clear though:

> Traversal of the pipeline source does not begin until the terminal operation 
> of the pipeline is executed.

and further on says:

> Spliterators for mutable data sources have an additional challenge; timing of 
> binding to the data, since the data could change between the time the 
> spliterator is created and the time the stream pipeline is executed. Ideally, 
> a spliterator for a stream would report a characteristic of IMMUTABLE or 
> CONCURRENT; if not it should be late-binding.

which explains why the collections in `java.util.concurrent` (whose 
spliterators have one of those characteristics) need not be late-binding.

-

PR: https://git.openjdk.java.net/jdk/pull/6275

Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs [v3]

[Anthony Vanelverdinghe]

On Wed, 10 Nov 2021 09:43:58 GMT, Hendrik Schreiber  
wrote:

>>> When I see ``, I'm just wondering what those <> type operators are 
>>> good for here...
>> 
>> What about just replacing `` with `...` then? The `State` 
>> constructor and its invocation also have an ellipsis.
>> 
>>> BUT, at least it's a working example and not some pseudo code.
>> 
>> The example is still not compilable due to the remaining ellipses.
>> 
>>> We do want to move to working example code long term, don't we?
>> 
>> I agree that examples should be compilable *in the raw Javadoc*. However, in 
>> the rendered Javadoc, using ellipses is a well-understood way to keep 
>> examples concise and devoid of irrelevant and/or usecase-dependent code. 
>> Moreover, when developers copy-paste the example, they'll immediately be 
>> pointed to all the places where they need to fill in the blanks, make a 
>> choice for a trade-off, etc. On the other hand, by hard-coding a 
>> (suboptimal) choice, developers who merely copy-paste the example are 
>> unlikely to reconsider the trade-off.
>> 
>>> The new example Cleaner instance _is_ shared, though on a pretty small 
>>> scale (just among instances of CleaningExample).
>> 
>> True, but my point was that the comment says "shared *within a library*". So 
>> to me it's confusing to have a comment saying "it's preferred to do A", and 
>> then have the code do B on the next line.
>> 
>> So I propose to either:
>> * revert the current change & simply replace `` with `...`
>> * update the comment to say: "A cleaner (preferably one shared within a 
>> library, but for the sake of example, a new one is created here)"
>> 
>> Actually, to have the line be compilable, and at the same time (attempt to) 
>> force developers to consider the trade-off, it could be changed to something 
>> like:
>> 
>> 
>> private static final Cleaner cleaner = createCleaner();
>> 
>> private static Cleaner createCleaner() {
>> // A cleaner, preferably one shared within a library
>> throw new UnsupportedOperationException("TBD");
>> }
>
> This is getting too complicated...
> 
> It's a code *example* with a very clear comment that explains a best practice:
> 
> // A cleaner, preferably one shared within a library
> private static final Cleaner cleaner = Cleaner.create();
> 
> 
> We cannot really show the best practice in this example without making the 
> example itself more complicated. IMHO, introducing an extra factory method 
> here adds nothing but complexity and makes the example more difficult to 
> understand (that aside, it should probably be something like 
> `MyLibrary.getCleaner()` and not a `createXyz()` method).
> 
> I still much more prefer `cleaner = Cleaner.create();` over `cleaner = 
> ` (which really is no better in any way, shape or form and creates 
> more questions than it provides answers) or `cleaner = ...`, which again does 
> not answer the question of how to get a cleaner instance—something I asked 
> myself when trying to use the API. In fact *how to get a cleaner instance* is 
> not explained by the current javadocs *at all* and it's something one *must* 
> know to use this API.
> 
> Here's our chance to show how it *can* be done, even if it's not ideal, 
> because it does not demonstrate the one-cleaner per library recommendation 
> (only mentions it).
> 
> And yes, those ellipsis in the `State` class code—I'd love for them to be 
> gone, too. It would make the example less abstract and easier to understand 
> (what's this state anyway? in most cases it's probably a reference to a 
> state, e.g. a native pointer, i.e. a `long`). But, admittedly, that's 
> difficult.
> 
> So, to summarize, there is always a tradeoff between making an example easy 
> to understand, not too complex, but still conveying the most important 
> information and best practices. In the end it's a matter of opinion. In this 
> case, I will stick to my original code change suggestion, because it adds 
> value (answers the question of how to get/create a cleaner instance).

In short: the current code


// A cleaner, preferably one shared within a library
private static final Cleaner cleaner = ;


is unhelpful to you, and the proposed code


// A cleaner, preferably one shared within a library
private static final Cleaner cleaner = Cleaner.create();


is confusing to me.

Trying to find a compromise, I'm merely asking that the comment be clarified:


// A cleaner (preferably one shared within a library, but for the sake of 
example, a new one is created here)
private static final Cleaner cleaner = Cleaner.create();


What do you think?

-

PR: https://git.openjdk.java.net/jdk/pull/6076

Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs

[Anthony Vanelverdinghe]

On Tue, 9 Nov 2021 08:59:32 GMT, Hendrik Schreiber  
wrote:

>> The new example Cleaner instance _is_ shared, though on a pretty small scale 
>> (just among instances of CleaningExample).  A demonstration of larger scale 
>> sharing of a Cleaner instance would be out of scope for this example.
>
> Let me add, why I have raised this issue.
> 
> I was going to migrate some older code which uses the `finalize()` method to 
> the `Cleaner` mechanism. New it it, there seemed to be two pitfalls:
> 
> 1. Understanding the whole "don't capture an instance reference in your state 
> object"
> 2. Copying the example (which was in a non-working state, due to pseudo code) 
> and making it work for me
> 
> With the improvement suggestions, I was trying help people who *only* read 
> the code sample (many do), to become aware of 1. and help them getting going 
> with 2, simply because it's something they can copy and run.

> When I see ``, I'm just wondering what those <> type operators are 
> good for here...

What about just replacing `` with `...` then? The `State` constructor 
and its invocation also have an ellipsis.

> BUT, at least it's a working example and not some pseudo code.

The example is still not compilable due to the remaining ellipses.

> We do want to move to working example code long term, don't we?

I agree that examples should be compilable *in the raw Javadoc*. However, in 
the rendered Javadoc, using ellipses is a well-understood way to keep examples 
concise and devoid of irrelevant and/or usecase-dependent code. Moreover, when 
developers copy-paste the example, they'll immediately be pointed to all the 
places where they need to fill in the blanks, make a choice for a trade-off, 
etc. On the other hand, by hard-coding a (suboptimal) choice, developers who 
merely copy-paste the example are unlikely to reconsider the trade-off.

> The new example Cleaner instance _is_ shared, though on a pretty small scale 
> (just among instances of CleaningExample).

True, but my point was that the comment says "shared *within a library*". So to 
me it's confusing to have a comment saying "it's preferred to do A", and then 
have the code do B on the next line.

So I propose to either:
* revert the current change & simply replace `` with `...`
* update the comment to say: "A cleaner (preferably one shared within a 
library, but for the sake of example, a new one is created here)"

Actually, to have the line be compilable, and at the same time (attempt to) 
force developers to consider the trade-off, it could be changed to something 
like:


private static final Cleaner cleaner = createCleaner();

private static Cleaner createCleaner() {
// A cleaner, preferably one shared within a library
throw new UnsupportedOperationException("TBD");
}

-

PR: https://git.openjdk.java.net/jdk/pull/6076

Re: RFR: 8276700: Improve java.lang.ref.Cleaner javadocs

[Anthony Vanelverdinghe]

On Fri, 22 Oct 2021 08:03:34 GMT, Hendrik Schreiber  
wrote:

> Trivial improvement.
> 
> Explicitly show how to create a `Cleaner` instance using `Cleaner.create()`.
> Repeat (again) in the code example that the `State` `Runnable `should be 
> implemented as static class and not reference the instance to be cleaned, to 
> make the point even more clear to those people who never read the javadoc 
> *prose*.
> 
> I have signed the OCA a while back as 
> [hschreiber](https://openjdk.java.net/census#hschreiber).

src/java.base/share/classes/java/lang/ref/Cleaner.java line 90:

> 88:  * public class CleaningExample implements AutoCloseable {
> 89:  *// A cleaner, preferably one shared within a library
> 90:  *private static final Cleaner cleaner = Cleaner.create();

Now the code (creating a private instance) goes against what the comment 
advises (using a shared instance), doesn't it?

-

PR: https://git.openjdk.java.net/jdk/pull/6076

Re: RFR: JDK-8276650: GenGraphs does not produce deterministic output

[Anthony Vanelverdinghe]

On Thu, 4 Nov 2021 19:28:58 GMT, Mandy Chung  wrote:

> The dot graph should print the edges in alphatical order.   A simple fix to 
> sort the edges before printing them.

src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ModuleDotGraph.java line 336:

> 334: private final String name;
> 335: private final Graph graph;
> 336: private final TreeSet descriptors = new 
> TreeSet<>();

This change is unnecessary, isn't it?

-

PR: https://git.openjdk.java.net/jdk/pull/6266

Re: RFR: 8266846: Add java.time.InstantSource

[Anthony Vanelverdinghe]

On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne  
wrote:

> 8266846: Add java.time.InstantSource

A nice addition to the JDK, thanks for taking this on.

src/java.base/share/classes/java/time/Clock.java line 114:

> 112:  * provides access to the current instant, and does not provide access 
> to the time-zone.
> 113:  * Where an application only requires the current instant {@code 
> InstantSource} should
> 114:  * be preferred to this class. For example, his might be the case where 
> the time-zone

his -> this

src/java.base/share/classes/java/time/Clock.java line 513:

> 511:  * {@link System#currentTimeMillis()} or equivalent.
> 512:  */
> 513: static final class SystemInstantSource implements InstantSource, 
> Serializable {

Is it possible to declare this as an enum? As doing so would simplify its 
implementation.

src/java.base/share/classes/java/time/InstantSource.java line 59:

> 57:  *  }
> 58:  * 
> 59:  * This approach allows an alternate source, such as {@link 
> #fixed(Instant) fixed}

alternate -> alternative? To me (being a non-native speaker) the latter reads 
more naturally

src/java.base/share/classes/java/time/InstantSource.java line 62:

> 60:  * or {@link #offset(InstantSource, Duration) offset} to be used during 
> testing.
> 61:  * 
> 62:  * The {@code system} factory method provide a source based on the best 
> available

provide -> provides

src/java.base/share/classes/java/time/InstantSource.java line 63:

> 61:  * 
> 62:  * The {@code system} factory method provide a source based on the best 
> available
> 63:  * system clock This may use {@link System#currentTimeMillis()}, or a 
> higher

missing dot after "clock"

src/java.base/share/classes/java/time/InstantSource.java line 175:

> 173: /**
> 174:  * Obtains a source that returns instants from the specified source 
> with the
> 175:  * specified duration added

missing dot to end the sentence

-

PR: https://git.openjdk.java.net/jdk/pull/4016

Re: ReversibleCollection proposal

[Anthony Vanelverdinghe]

Hi Stuart

Will EnumSet also be updated to implement ReversibleSet? Or will it be updated 
to implement NavigableSet [1] independently of this enhancement?

I'd also like to propose adding `ReversibleSet::of` factory methods. This is 
something I miss having on SortedSet occasionally, but ReversibleSet would 
actually be a better fit for such methods.

Kind regards, Anthony

[1] https://bugs.openjdk.java.net/browse/JDK-6278287

On Friday, April 16, 2021 19:40 CEST, Stuart Marks  
wrote:

> This is a proposal to add a ReversibleCollection interface to the Collections
> Framework. I'm looking for comments on overall design before I work on 
> detailed
> specifications and tests. Please send such comments as replies on this email 
> thread.
>
> Here's a link to a draft PR that contains the code diffs. It's prototype 
> quality,
> but it should be good enough to build and try out:
>
>  https://github.com/openjdk/jdk/pull/3533
>
> And here's a link to a class diagram showing the proposed additions:
>
>
> https://cr.openjdk.java.net/~smarks/ReversibleCollection/ReversibleCollectionDiagram.pdf
>
> Thanks,
>
> s'marks
>
>
> # Ordering and Reversibility
>
>
> A long-standing concept that's been missing from collections is that of the
> positioning, sequencing, or arrangement of elements as a structural property 
> of a
> collection. (This is sometimes called the "iteration order" of a collection.) 
> For
> example, a HashSet is not ordered, but a List is. This concept is mostly not
> manifested in the collections API.
>
> Iterating a collection produces elements one after another in *some* 
> sequence. The
> concept of "ordered" determines whether this sequence is defined or whether 
> it's a
> coincidence of implementation. What does "having an order" mean? It implies 
> that
> there is a first element and that each element has a successor. Since 
> collections
> have a finite number of elements, it further implies that there is a last 
> element
> that has no successor. However, it is difficult to discern whether a 
> collection has
> a defined order. HashSet generally iterates its elements in the same undefined
> order, and you can't actually tell that it's not a defined order.
>
> Streams do have a notion of ordering ("encounter order") and this is 
> preserved,
> where appropriate, through the stream pipeline. It's possible to detect this 
> by
> testing whether its spliterator has the ORDERED characteristic. Any 
> collection with
> a defined order will have a spliterator with this characteristic. However, 
> this is
> quite a roundabout way to determine whether a collection has a defined order.
> Furthermore, knowing this doesn't enable any additional operations. It only 
> provides
> constraints on the stream's implementations (keeping the elements in order) 
> and
> provides stronger semantics for certain operations. For example, findFirst() 
> on an
> unordered stream is the same as findAny(), but actually finds the first 
> element if
> the stream is ordered.
>
> The concept of ordering is thus present in the system but is surfaced only in 
> a
> fairly indirect way. We can strengthen abstraction of ordering by making a few
> observations and considering their implications.
>
> Given that there is a first element and a last element, the sequence of 
> elements has
> two ends. It's reasonable to consider operations (add, get, remove) on either 
> end.
> Indeed, the Deque interface has a full complement of operations at each end. 
> This is
> an oft-requested feature on various other collections.
>
> Each element except for the last has a successor, implying that each element 
> except
> for the first has a predecessor. Thus it's reasonable to consider iterating 
> the
> elements from first to last or from last to first (that is, in forward or 
> reverse
> order). Indeed, the concept of iterating in reverse order appears already in 
> bits
> and pieces in particular places around the collections:
>
>   - List has indexOf() and lastIndexOf()
>   - Deque has removeFirstOccurrence() and removeLastOccurrence()
>   - List has a ListIterator with hasPrevious/previous methods
>   - Deque and NavigableSet have descendingIterator methods
>
> Given an ordered collection, though, there's no general way to iterate it in 
> reverse
> order. Reversed iteration isn't the most common operation, but there are some
> frequent use cases, such as operating on elements in most-recently-added 
> order.
> Questions and bug reports about this have come up repeatedly over the years.
>
> Unfortunately, iterating in reverse order is much harder than iterating in 
> forward
> order. There are a variety of ways to iterate in forward order. For example, 
> given a
> List, one can do any of the following:
>
>  for (var e : list) { ... }
>  list.forEach(...)
>  list.stream()
>  list.toArray()
>
> However, to iterate a list in reverse order, one must use an explicit loop 
> over
> ListIterator:
>
>  for (var it = 

Re: [External] : Re: ReversibleCollection proposal

[Anthony Vanelverdinghe]

Hi Rémi

The discussion "new types? or new default methods?" is indeed a valid one. In 
fact, I think this choice has already been made once in favor of a default 
method, when adding `List::sort` was chosen over adding `SortedList` (though I 
imagine that choice was a no-brainer).

In this case I prefer adding new types though, so I wanted to share my take on 
this.

In the following diagram (see [1] in case it got mangled), I've tried to 
arrange all relevant Collection types by their defining characteristics:

|- Collection 
---|
|unorderedordered   sorted   
distinctdistinct & ordereddistinct & sorted |
|Set
 |
| Queue = (get|remove)First + addLast   PriorityQueue   
 |
| Stack = (add|get|remove)Last  
 |
| Deque = Queue + Stack + addFirst  
 LinkedHashSet SortedSet |
| List = Deque + (add|get|remove)IndexedList::sort  
   NavigableSet  |
||

As I see it, there are a couple of issues with this:
* conceptually, every List is a Deque, but List doesn't extend Deque. If it 
would, all uses of List (e.g. as a parameter type in APIs) where the indexed 
access isn't required could be replaced with Deque instead. Or e.g. when you 
need to take a stack as argument: currently Deque is the recommended parameter 
type, but that means you can't just pass a List to the method as-is. With RC, 
you'd be able to use that as parameter type and pass both Deque and List.
* LinkedHashSet and SortedSet lack a common ancestor which asserts "this is an 
ordered set". So when defining an API, I'm forced to choose between SortedSet, 
which is more specific than I want, or List, which is more generic than I want 
(usually I go with SortedSet, because enforcing something through Javadoc isn't 
very reliable (cf. Collectors::toList: even though the spec clearly says the 
result might not be modifiable, people tend to simply assume it is)).

Now with RC/RS these issues would be solved, where RC is ~ Deque + reversed, 
and RS ~ Deque + distinct + reversed. In terms of the diagram, they group 
together a bunch of closely-related Collection types (see [1] if needed):

|- Collection 
---|
|unorderedordered   sorted   
distinctdistinct & ordereddistinct & sorted |
|Set
 |
| Queue = (get|remove)First + addLast   PriorityQueue   
 |
| Stack = (add|get|remove)Last  
 |
||- ReversibleCollection ---|   
|- ReversibleSet ---||
||Deque = Queue + Stack + addFirst  |   
|LinkedHashSet SortedSet||
||List = Deque + (add|get|remove)IndexedList::sort  |   
|  NavigableSet ||
||--|   
|---||
||

On Wednesday, May 12, 2021 13:22 CEST, fo...@univ-mlv.fr wrote:

> First, i think we have overlooked ReversibleMap, if you have a LinkedHashMap, 
> the keySet should be a ReversibleSet.

I'm not sure what you meant, but the PR already defines `LinkedHashMap::keySet` 
as `public ReversibleSet keySet()`.

> Again, we have seen that introducing those interfaces
> - is not source backward compatible thus it will be painful for some of our 
> users,
>   I believe NavigableSet/NavigableMap did not have that issue because only 
> one existing implementation per interface was retrofitted to implement those 
> interfaces, TreeSet for NavigableSet and TreeMap for NavigableMap.
>   ConcurrentSkipListSet/ConcurrentSkipListMap were added at the same time, so 
> there was no existing code doing a lub (lowest upper bound) between TreeSet 
> and ConcurrentSkipListSet.
>   Here, they are a lot of implementations that will implement be retrofitted 
> to 

Re: ReversibleCollection proposal

[Anthony Vanelverdinghe]

On Tuesday, April 27, 2021 11:25 CEST, Peter Levart  
wrote:
> Now just some of my thoughts about the proposal:
>
> - SortedSet.addFirst/.addLast - I think an operation that would be used
> in situations like: "I know this element should always be greater than
> any existing element in the set and I will push it to the end which
> should also verify my assumption" is a perfectly valid operation. So 
> those methods throwing IllegalStateException in case the invariant can't
> be kept seem perfectly fine to me.

This was raised before and addressed by Stuart in [0]:
"An alternative as you suggest might be that SortedSet::addFirst/addLast could 
throw
something like IllegalStateException if the element is wrongly positioned.
(Deque::addFirst/addLast will throw ISE if the addition would exceed a capacity
restriction.) This seems error-prone, though, and it's easier to understand and
specify that these methods simply throw UOE unconditionally. If there's a good 
use
case for the alternative I'd be interested in hearing it though."

> - ReversibleCollection.addFirst/.addLast are specified to have void
> return type. This works for List and Deque which always add element and
> so have no information to return. OTOH Collection.add is specified to
> return boolean to indicate whether collection was modified or not, but
> Set modifies the specification of that method a bit to be: return false
> or true when Set already contained an element equal to the parameter or
> not. So ReversibleCollection.addFirst/.addLast could play the same
> trick. for List(s) and Deque(s) it would always return true, but for 
> ReversibleSet(s) it would return true only if the set didn't contain an
> element equal to the parameter, so re-positioning of equal element would
> return false although the collection was modified as a result.

If addFirst/addLast were to return boolean, Deque would no longer compile due 
to its existing methods being incompatible with the new ones.

> Regards, Peter

Kind regards, Anthony

[0] https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-April/076518.html

Re: ReversibleCollection proposal

[Anthony Vanelverdinghe]

On Saturday, April 17, 2021 11:48 CEST, Stephen Colebourne 
 wrote:

> On Fri, 16 Apr 2021 at 18:41, Stuart Marks  wrote:
> > This is a proposal to add a ReversibleCollection interface to the 
> > Collections
> > Framework. I'm looking for comments on overall design before I work on 
> > detailed
> > specifications and tests. Please send such comments as replies on this 
> > email thread.
>
> I think this could be an interesting addition to the framework.
>
> > # Ordering and Reversibility
>
> Reading this section, it seems like ordering is a more significant
> quality than reversibility. Yet the API is named "reversible". That
> seems odd, esepcially given the stream characteristic.

Since `reversible = ordered + sized`, using reversible makes sense imho (even 
though in the context of the Collections Framework, reversible <-> ordered, 
since all collections are sized). Also, I think "reversible" is much less 
likely to clash with existing code (e.g. a quick GitHub search shows 1 result 
for `ReversibleCollection`, while there's plenty for `OrderedCollection`).

>
> > SortedSet::addFirst and addLast throw UnsupportedOperationException. This 
> > is because
> > SortedSet's ordering is determined by the comparison method and cannot be 
> > controlled
> > explicitly.
>
> This seems undesirable. Maybe SortedSet should not implement
> reversible/ordered? Maybe they should add to the set but validate
> whether they would be in first/last position? Simply allowing users to
> get a new instance with a different (eg. reversed) comparator would
> meet much of the use case.

I'd argue throwing UOE is the right thing to do. Not having `SortedSet` 
implement `ReversibleSet` doesn't make sense to me. Adding with validation is 
reasonable in itself, but then you'd have to specify `IllegalArgumentException` 
in `ReversibleCollection` (where you'd have a hard time specifying the 
conditions under which it might be thrown without explicitly referencing 
`SortedSet`), just to accommodate these 2 methods which would be very rarely 
used.

>
> Also, SortedSet uses first() and last(), yet the proposed interface
> uses getFirst() and getLast().

Since `Deque` uses `getFirst()` and `getLast()`, it's impossible to match all 
existing methods in the different interfaces.

>
> Stephen

Kind regards, Anthony

Re: Insufficiencies in JEP: 400: UTF-8 by Default

[Anthony Vanelverdinghe]

Hi Alan

As Marco mentioned, another use case is sub-process stdin/stdout/stderr. In my 
particular instance, I'm starting a Process which has its output redirected to 
a file. It uses the platform's default encoding for writing to stdout. So when 
I want to read its output from the file at some later point, I need to supply 
that encoding to the Files API.
One way to accommodate this use case, is a method which allows to retrieve the 
platform's default encoding, for example a method `platformEncoding` in Charset 
or Process, or the `Console::charset` method you mentioned. Another option 
would be to enhance the Process API, by adding methods to Process which return 
appropriate Readers/Writers & adding methods of the form `redirectX(File file, 
Charset encoding)` to ProcessBuilder. But this seems like a lot of additional 
API surface, just to avoid surfacing the platform's default encoding itself.
So I think the JEP should specify how it'll address use cases w.r.t. the 
Process API, shouldn't it?

Kind regards,
Anthony

On Sunday, March 14, 2021 13:01 CET, Alan Bateman  
wrote:

> On 14/03/2021 11:00, Marco wrote:
> > :
> >
> > IMO Charset should provide standardized getters for the OS charset and the
> > console charset. The latter being different has been a long standing issue 
> > on
> > Windows where the codepage differs between its CLI and regular environments.
> > OpenJDK has the necessary data already available in its custom system
> > properties.
> >
> > The console charset is currently hidden behind PrintStream not exposing the
> > underlying OSWriter and not offering getEncoding() itself. The OS charset
> > would be hidden in the future by Charset.getDefaultCharset()'s specification
> > change in JEP 400.
> The intention that there will be little or no impact to the console
> streams. This means that java.io.Console reader/writer methods should
> continue to return a Reader/PrintWriter that uses the platform encoding
> (or code page is on Windows). Same thing for the System.out/System.err
> print streams. We need to make this clearer in the JEP.
>
> There has been discussion on this mailing list about adding a
> Console::charset method but it didn't come to a consensus. Naoto Sato
> and I have been chatting about it again recently as there may be a need
> to add an API in advance of proposing to target the JEP.
>
> One case that we are still mulling over is code that creates an
> InputStreamReader on System.in without specifying the charset. This
> might be older code that pre-dates java.io.Console or maybe code that
> wasn't tested on a wide range or platforms. Options range from a spec
> change to doing nothing (the latter meaning running with "COMPACT" or
> migrating the code to use the 2-arg constructor as the default charset
> is not the right choice).
>
> -Alan
>
>
>

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[Anthony Vanelverdinghe]

Hi Julia

Since short-circuiting sounds similar to a Subscriber cancelling its 
Subscription, I believe it might be worthwhile to consider the Flow API.
If the argument would be a `Flow.Processor`, then the 
implementation would publish instances of T to it, subscribe to receive 
instances of R, and be able to short-circuit by cancelling its Subscription.


Disclaimer: I haven't actually prototyped this, but it makes sense at 
first thought


Kind regards,
Anthony

On 03/07/2020 11:00, Julia Boes wrote:

Hi Tagir,

By the way, the proposed API allows no possibility to short-circuit
the pusher. So if mapMulti produces many elements and short-circuiting
terminal operation like anyMatch finds the match, it won't stop the
pusher from pushing. Have you considered to use
BiConsumer, T>, so Stream API may signal to the pusher
that it's ok to stop pushing? Note that in modern Java versions,
flatMap can actually short-circuit.


We did prototype mapMulti with a Predicate and it brought to light 
some limitations that led us to decide against this approach.


1) On the user side, Predicate::test or Predicate::negate would be 
used to check if it's ok to keep pushing. The semantics of these 
methods doesn't really fit the use case (short-circuiting requested 
true/false) and we would probably need to come up with a new method to 
avoid confusion.


2) Along the lines of 1), but more generally the burden of exposing an 
implementation detail to the user. You are right that flatMap supports 
short-circuiting, but it's hidden in the implementation and the user 
doesn't need to know how it's implemented or how to make it work. For 
mapMulti however, we would need to expose some of it through the use 
of Predicate, and explain to the user how to use it correctly.


3) While the JDK implementation uses Sink, a type of Consumer that 
supports cancellation, the default implementation uses SpinedBuffer, a 
type of Consumer that does not support cancellation. We would need to 
add that functionality.


4) The difficulty of signalling a short-circuit request across the 
stream pipeline, in particular because mapMulti uses flatMap 
internally, which creates something like an inner nested pipeline. 
While it's possible to signal across the main outer pipeline, it's not 
easy to signal between outer and inner pipeline.



Regards,

Julia

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[Anthony Vanelverdinghe]

On 02/07/2020 16:38, Patrick Concannon wrote:

Hi Anthony,

Hi Patrick

Thanks for your suggestion of using a Stream.Builder instead of a Consumer.
Thanks for your response. Since Stream.Builder extends Consumer, I don't 
see it as "instead of": the proposed signature is a strict superset of 
the actual signature. For example, it would accept a BiConsumerStream.Builder>, whereas the current signature doesn't.

However, one of the goals for mapMulti is not to create an additional Stream.
Would you mind to elaborate on this? Given that Stream.Builder extends 
Consumer, I don't see why using Stream.Builder would imply having to 
create any more Streams than when using Consumer.

Also, I fear that using a builder, but throwing an exception on a call to 
build, is counter-intuitive and perhaps confusing to the user.
Point taken, but I beg to differ: to me this is no different than a 
method `void foo(Consumer bar)` which specifies that the 
given Consumer must not invoke `close` on the InputStream.
Moreover, with the build step being the sole difference with `flatMap`, 
and the elimination of intermediary Streams being a rather obvious 
optimization, I believe most users would find it intuitive not to invoke 
`build` themselves (or they would have read the Javadoc and know not to :)).
Finally, using Stream.Builder makes it intuitive for me what the method 
does, even without considering its name.


Given the above, I still hold that Stream.Builder is a better choice to 
use in the signature. Even more so, I believe there are several things 
that hint at the need for an additional abstraction: your point about 
the potential confusion, the analogy with Collector (as already 
mentioned by Paul w.r.t. the order of the BiConsumer's type arguments), 
and the natural similarity between e.g. BiConsumer> 
and Function>.


Therefore I'd like to propose the following (complete declarations below):

* introduce a new interface: Transformer extends Function
* adapt Collector to extend it: Collector extends 
Transformer, A, R>


So a Transformer is nothing more than a Function which passes via an 
intermediary I to map T to R according to a certain protocol.


Then a method with the following signature could be added to Stream:
 Stream flatMap(Transformer, 
? extends Stream> mapper)


Such a Transformer could readily be created from a given BiConsumer (see 
Tranformer::of below).


Since Transformer is a subinterface of Function, there wouldn't be any 
ambiguities w.r.t. overload resolution, so we could just name the new 
method `flatMap` (I assume such ambiguities are the reason why this 
currently isn't the case, since Function and BiConsumer are unrelated?).


Below is a self-contained "demo", containing a simple Demo class, the 
declaration of Transformer, and the adapted declaration of Collector.


Thanks in advance for any feedback.

Kind regards
Anthony



import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.function.BiConsumer;
import java.util.function.BinaryOperator;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Stream;

public class Demo {

    public static void main(String[] args) {
    BiConsumer> anagrams = (word, 
builder) -> {

    if(word.equals("listen")) {
    builder.accept("silent");
    builder.accept("enlist");
    }
    };
Transformer.of(anagrams::accept).apply("listen").forEachOrdered(System.out::println);
    }

}

interface Transformer extends Function {

    Supplier supplier();

    BiConsumer transformer();

    Function finisher();

    @Override
    default R apply(T t) {
    var intermediary = supplier().get();
    transformer().accept(t, intermediary);
    return finisher().apply(intermediary);
    }

    static  Transformer, Stream> 
of(BiConsumer> transformer) {

    return new Transformer<>() {
    @Override
    public Supplier> supplier() {
    return Stream::builder;
    }

    @Override
    public BiConsumer> transformer() {
    return transformer::accept;
    }

    @Override
    public Function, Stream> finisher() {
    return Stream.Builder::build;
    }
    };
    }

}

interface Collector extends Transformer, A, R> {

    @Override
    Supplier supplier();

    BiConsumer accumulator();

    BinaryOperator combiner();

    @Override
    Function finisher();

    Set characteristics();

    enum Characteristics {
    CONCURRENT,
    UNORDERED,
    IDENTITY_FINISH
    }

    @Override
    default BiConsumer, A> transformer() {
    return (stream, intermediary) -> {
    var accumulator = accumulator();
    stream.forEachOrdered(t -> accumulator.accept(intermediary, 
t));

 

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

[Anthony Vanelverdinghe]

Hi

Given the signature of flatMap is:
 Stream flatMap​(FunctionR>> mapper)


I'd like to propose the following signature for the new method:
 Stream builderMap(BiConsumerStream.Builder> mapper)


This way both methods are named "...Map", and the name "builderMap" 
follows naturally from the argument's type.
If the given mapper invokes Stream.Builder::build, an 
IllegalStateException should be thrown.


Kind regards,
Anthony

On 25/06/2020 02:58, Paul Sandoz wrote:

Hi,

We traded CPS style for reusing existing functional interfaces. Originally the 
signature (my first choice) was as you indicate.

By chance it just so happens that the current signature is the same shape as 
that for the accumulating argument type of the three arg collect terminal 
operation:

Stream
 R collect(Supplier supplier,
   BiConsumer accumulator,
   BiConsumer combiner);

IntStream
 R collect(Supplier supplier,
   ObjIntConsumer accumulator,
   BiConsumer combiner);

Same for the accumulator of a Collector too.

However, I suspect you would argue these terminal accumulation cases are 
different from the intermediate case, as we are not accumulating but passing or 
accepting (loosely returning) zero or more elements that replace the input 
element.

It’s my hope that generic specialization will allow the primitive stream types 
to fade into the background, along with the primitive functional interfaces. In 
that respect the addition of three functional interfaces for use on the 
primitive stream types is not so terrible.


Regarding the name, you should have seen the first one :-) it was terrible.

Here’s my few brush strokes on the bike shed. I wonder what people think of mapAccept. The specification 
talks about accepting elements, because that is the operative method name on Consumer. So we can say 
"T is replaced with the elements accepted by the Consumer", or “ The Consumer 
accepts the elements that replace T"

Paul.




On Jun 24, 2020, at 1:01 PM, John Rose  wrote:

I like this new API point a lot; it allows flexible, local, temporary
control inversion in the context of one stream transform.

What’s the performance model?  It seems like the SpinedBuffer
implementation makes a worst-case assumption about the number
of pending values, that there will be many instead of zero or one.

But I guess the pipeline stuff already works in terms of pushes, so
the good news might be that this is really just a drill-down from the
user API into the kinds of operations (push-flavored) that go on
most of the time.

OK, so I like the function but I have a beef with its bike shed
color.  First of all, this is a control-inversion (CPS) pattern,
which is very powerful but also notoriously hard to read.
I think that in Java APIs, at least in Stream APIs, code is
easier to read if the logical data flow is from left to right.

(This is a language-specific observation.  Apart from varargs,
Java method APIs read favorably when extra control arguments
are added onto the end of the argument list.  Also, the convention
for generic functional interfaces is that the return value type
goes to the right, e.g., R in Function.)

So the BiConsumer is backwards, because the logical return
should be written, if not as a true return (which would appear
at the end of type parameter lists), at the end of the incoming
parameters (and in the last type parameter).

I also think “multi” is needlessly “learned” sounding.  A simple
spatial preposition would work well: mapThrough, mapAcross, etc.
I think I prefer mapAcross because the term “across” can be read
adverbially: “we are mapping T across to Consumer”.

So:

mapAcross(BiConsumer> mapper)
mapAcrossToInt(BiConsumer mapper)
mapAcross​(IntObjConsumer mapper)

This does require additional FI’s like IntObjConsumer, but
I think that is a negligible cost.  Making the control inversion
*readable* is the high order bit here, not minimizing the number
of trivial FIs.

(I almost hear David Bowman, in his space suit, saying, “My API…
It’s full of bikesheds!”  There’s a meme for that.)

— John

On Jun 24, 2020, at 3:57 AM, Patrick Concannon  
wrote:

Hi,

Could someone please review myself and Julia's RFE and CSR for JDK-8238286 - 
'Add new flatMap stream operation that is more amenable to pushing’?

This proposal is to add a new flatMap-like operation:

` Stream mapMulti(BiConsumer, ? super T> mapper)`

to the java.util.Stream class. This operation is more receptive to the pushing 
or yielding of values than the current implementation that internally assembles 
values (if any) into one or more streams. This addition includes the primitive 
variations of the operation i.e. mapMultiToInt, IntStream mapMulti, etc.

issue: https://bugs.openjdk.java.net/browse/JDK-8238286 

csr: https://bugs.openjdk.java.net/browse/JDK-8248166 


webrev: 

Trivial patches for typos in java.base

[Anthony Vanelverdinghe]

Hi

I filed 2 bug reports for typos in java.base Javadoc:

https://bugs.openjdk.java.net/browse/JI-9064752 with patch

diff --git 
a/src/java.base/share/classes/java/lang/invoke/package-info.java 
b/src/java.base/share/classes/java/lang/invoke/package-info.java

index 82600d4866..a0ad09b03e 100644
--- a/src/java.base/share/classes/java/lang/invoke/package-info.java
+++ b/src/java.base/share/classes/java/lang/invoke/package-info.java
@@ -31,7 +31,7 @@
  * As described in the Java Virtual Machine Specification, certain 
types in this package

  * are given special treatment by the virtual machine:
  * 
- * The classes {@link java.lang.invoke.MethodHandle MethodHandle}
+ * The classes {@link java.lang.invoke.MethodHandle MethodHandle} and
  * {@link java.lang.invoke.VarHandle VarHandle} contain
  * signature polymorphic methods
  * which can be linked regardless of their type descriptor.
@@ -190,7 +190,7 @@
  * invoked with just the parameter types of static arguments, thereby 
supporting a wider
  * range of methods compatible with the static arguments (such as 
methods that don't declare

  * or require the lookup, name, and type meta-data parameters).
- *  For example, for dynamically-computed call site, a the first 
argument
+ *  For example, for a dynamically-computed call site, the first 
argument
  * could be {@code Object} instead of {@code MethodHandles.Lookup}, 
and the return type

  * could also be {@code Object} instead of {@code CallSite}.
  * (Note that the types and number of the stacked arguments limit

and https://bugs.openjdk.java.net/browse/JI-9064753 with patch

diff --git 
a/src/java.base/share/classes/java/util/concurrent/locks/ReentrantLock.java 
b/src/java.base/share/classes/java/util/concurrent/locks/ReentrantLock.java

index 415e50e29c..be46131d3e 100644
--- 
a/src/java.base/share/classes/java/util/concurrent/locks/ReentrantLock.java
+++ 
b/src/java.base/share/classes/java/util/concurrent/locks/ReentrantLock.java

@@ -84,7 +84,7 @@ import jdk.internal.vm.annotation.ReservedStackAccess;
  * try {
  *   // ... method body
  * } finally {
- *   lock.unlock()
+ *   lock.unlock();
  * }
  *   }
  * }}

There's also a typo in a comment inside native code, but I didn't file a 
bug report for it:


diff --git a/src/java.base/unix/native/libjava/ProcessImpl_md.c 
b/src/java.base/unix/native/libjava/ProcessImpl_md.c

index 9c562a3d16..68aa1c457a 100644
--- a/src/java.base/unix/native/libjava/ProcessImpl_md.c
+++ b/src/java.base/unix/native/libjava/ProcessImpl_md.c
@@ -93,7 +93,7 @@
  *    grained control about how exactly the process fork is executed. 
It is

  *    powerful, but Linux-specific.
  *
- * Aside from these three possibilities there is a forth option: 
posix_spawn(3).
+ * Aside from these three possibilities there is a fourth option: 
posix_spawn(3).
  * Where fork/vfork/clone all fork off the process and leave pre-exec 
work and
  * calling exec(2) to the user, posix_spawn(3) offers the user 
fork+exec-like

  * functionality in one package, similar to CreateProcess() on Windows.

Kind regards,
Anthony

RE: Suggestion for a more sensible implementation of EMPTY_MAP

[Anthony Vanelverdinghe]

Hi Abraham



Wouldn’t it have been possible to use Map::isEmpty? Ideally just returning at 
the very beginning of your method if `data` is an empty Map. That way you’d 
still be able to use computeIfPresent, and use emptyMap() as well.



Kind regards,

Anthony




From: core-libs-dev  on behalf of 
Abraham Marín Pérez 
Sent: Wednesday, June 19, 2019 9:51:46 AM
To: Roger Riggs
Cc: core-libs-dev@openjdk.java.net
Subject: Re: Suggestion for a more sensible implementation of EMPTY_MAP

Hi Stuart, Roger,

First of all, thank you for such a detailed response, this really shows the big 
picture. I guess no implementation will be perfect, there will always be some 
wrinkles that we need to accept, and the most suitable implementation will be 
the one with the fewest or least impacting wrinkles. Given this, maybe showing 
the particular wrinkle that I faced can shed light on the overall development 
experience that the current implementation provides.

For multiple reasons that I cannot explain at this point, I have a method that 
looks like the following:

private void decorate(Map data) {
//...
data.computeIfPresent("field", (k, v) -> highlightDifferences(v, 
otherValue));
//...
}

At one point an emptyMap() was passed to this method, causing an UOE. This left 
me with two choices:

1. Change code to:

if(data.hasKey(“field"))
data.compute("field", (k, v) -> highlightDifferences(v, otherValue));

2. Avoid usage of emptyMap() and use new HashMap<>() instead


The first option defeats the purpose of having a computeIfPresent method, and 
causes confusion among team members (people keep asking why computeIfPresent 
isn't used). The second option is pretty much what Roger mentioned (and what we 
ended up doing).

On the other hand, there are some points that you mentioned that I believe 
merit some extra debate, please find some comments below inline.

In general, I tend to agree that a stricter implementation is better since it 
usually prevents bugs. However, at least in this particular case, I believe a 
stricter implementation also has the side effect of worsening the development 
experience and adding confusion (see below comments). Of course, as I mentioned 
before, I understand that no implementation will be perfect, and maybe the 
current option is the lesser evil, but I wanted to throw in an alternative for 
consideration.

Many thanks,
Abraham


El vie., 14 jun. 2019 a las 14:18, Roger Riggs (mailto:roger.ri...@oracle.com>>) escribió:
Hi Stuart,

Not withstanding all the details. It would be useful (and possibly
expected) that an EMPTY_MAP
could be substituted for a Map with no entries.  As it stands, the
caller needs know whether any optional
possibly modifying operations will be used and decide whether to create
an empty map or an EMPTY_MAP.
That makes using an EMPTY_MAP a risk and less useful and a cautious
programmer will avoid it.

$.02, Roger


On 6/13/19 8:42 PM, Stuart Marks wrote:
>
>
> On 6/10/19 7:34 AM, Abraham Marín Pérez wrote:
>> I agree that it makes sense for EMPTY_MAP to have the same logic as
>> Map.of() or unmodifiable(new HashMap()), which means that my
>> suggestion cannot be applied to just EMPTY_MAP... but maybe that’s
>> ok: maybe we can change all of them? If we keep the default
>> implementation for Map.computeIfPresent in EMPTY_MAP, Map.of(),
>> unmodifiableMap, etc., instead of overriding it to throw an
>> exception, then:
>>
>> - For cases where the key isn’t present (regardless of empty or
>> non-empt map), the call will be a safe no-op.
>> - For cases where the key is present, the call will still throw UOE
>> via the implementation of put()
>>
>> I believe this would still respect the spec for Map.computeIfPresent
>> while providing a more forgiving implementation (in the sense that it
>> will avoid throwing an exception when possible).
>
> Hi Abraham,
>
> The specification does not mandate one behavior or another. Either
> behavior is permitted, and in fact both behaviors are present in the JDK.
>
> The second and more significant point is raised by your last statement
> suggesting a "more forgiving implementation." The question is, do we
> actually want a more forgiving implementation?
>
> The collections specs define certain methods as "optional", but it's
> not clear whether this means that calling such a method should always
> throw an exception ("strict" behavior) or whether the method should
> throw an exception only when it attempts to do something that's
> disallowed ("lenient" behavior).
>
> Take for example the putAll() method. What happens if we do this?
>
> Map map1 = Collections.emptyMap();
> map1.putAll(otherMap);
>
> The answer is that it depends on the state of otherMap. If otherMap is
> empty, then the operation succeeds (and does nothing). However, if
> otherMap is non-empty, then the operation will throw
> UnsupportedOperationException. That's an example of lenient behavior.
> What's notable about 

RE: [PATCH] 4511638: Double.toString(double) sometimes produces incorrect results

[Anthony Vanelverdinghe]

My bad. I was looking at the patch at [1], and didn’t consider that the browser 
rendering likely doesn’t match the actual file content. Sorry for the noise. 
And good to hear the review process is underway.

Kind regards,
Anthony

[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059783.html


From: Raffaello Giulietti 
Sent: Monday, June 17, 2019 4:56:22 PM
To: Anthony Vanelverdinghe; core-libs-dev
Subject: Re: [PATCH] 4511638: Double.toString(double) sometimes produces 
incorrect results


Hi Anthony,

On 16/06/2019 19.17, Anthony Vanelverdinghe wrote:

Hi Raffaello



While I don't have feedback on the actual math, here's a few suggestions:

- there's some use of non-ASCII characters in the patch. I don't think this is 
common in the JDK's Java sources, so you might want to replace them with their 
Unicode escapes. The characters are: ≤ (\u2264), ∞ (\u221e), × (\u00d7), ≥ 
(\u2265), … (\u2026), ≠ (\u2260), ⌊ (\u230a), ⌋ (\u230b), · (\u00b7), β (\u03b2)

I'm not sure what you mean here: there are indeed usages of HTML entities like 
 that appear as math symbols in the rendered Javadoc but the sources are 
100% US-ASCII in my IDE and should be so even in the patch.

If not, I'd be interested in the "coordinates" of the characters outside of the 
US-ASCII set.


- there are 2 invocations of a deprecated String constructor for performance 
reasons. I don't know how big the performance difference is, but I would 
suggest replacing these invocations with `new String(bytes, 
StandardCharsets.US_ASCII)` instead, and filing a bug for the performance 
difference with the deprecated constructor

The perf diff is measurable. The chosen variant is the fastest that I could 
come up with. As long as the deprecated constructor does not become "forRemoval 
= true", if at all, I wouldn't worry. Even then, there's plenty of time to 
switch.

Until then I can try to understand why the used constructor is faster and 
perhaps file a bug as you suggest.


- there are 2 occurrences of a typo "left-to-tight"

Yep, thanks for your eagle's eyes!




Other than that, I can only say this is an impressive piece of work, so I hope 
some official Reviewers will help you add your contribution to the JDK.

The review process has indeed begun some days ago. However, since there's is 
quite some maths to digest to understand the code, a full review could take 1 
to 2 days of concentrated reading: not the average review, I guess.

Plus, there's a change in the specification, which must undergo a further 
review.


Thanks for your time!

Greetings
Raffaello





Kind regards

Anthony




From: core-libs-dev 
<mailto:core-libs-dev-boun...@openjdk.java.net>
 on behalf of Raffaello Giulietti 
<mailto:raffaello.giulie...@gmail.com>
Sent: Monday, May 6, 2019 2:08:48 PM
To: core-libs-dev
Subject: [PATCH] 4511638: Double.toString(double) sometimes produces incorrect 
results

Hi,

no new code this time, only a warm invitation to review the rather
substantial patch submitted on 2019-04-18 [1] and the CSR [2].

I spent an insane amount of (unpaid) free time offering my contribution
to solve a bug known since about 15 years. Thus, I think it is
understandable that I'd like to see my efforts come to a successful
conclusion, ideally for OpenJDK 13.


Greetings
Raffaello


P.S.
Some enjoyable properties of the novel algorithm:
* No objects are instantiated, except, of course, the resulting String.
* Loop-free core algorithm.
* Only int and long arithmetic.
* No divisions at all.
* 16x speedup (jmh).
* Randomized, yet reproducible deep diving tests (jtreg).
* Clear, unambiguous spec.
* All floats have been tested to fully meet the spec.
* Fully documented in [3] or in comments.



[1]
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059783.html
[2] https://bugs.openjdk.java.net/browse/JDK-8202555
[3] https://drive.google.com/open?id=1KLtG_LaIbK9ETXI290zqCxvBW94dj058

RE: [PATCH] 4511638: Double.toString(double) sometimes produces incorrect results

[Anthony Vanelverdinghe]

Hi Raffaello



While I don't have feedback on the actual math, here's a few suggestions:

- there's some use of non-ASCII characters in the patch. I don't think this is 
common in the JDK's Java sources, so you might want to replace them with their 
Unicode escapes. The characters are: ≤ (\u2264), ∞ (\u221e), × (\u00d7), ≥ 
(\u2265), … (\u2026), ≠ (\u2260), ⌊ (\u230a), ⌋ (\u230b), · (\u00b7), β (\u03b2)

- there are 2 invocations of a deprecated String constructor for performance 
reasons. I don't know how big the performance difference is, but I would 
suggest replacing these invocations with `new String(bytes, 
StandardCharsets.US_ASCII)` instead, and filing a bug for the performance 
difference with the deprecated constructor

- there are 2 occurrences of a typo "left-to-tight"



Other than that, I can only say this is an impressive piece of work, so I hope 
some official Reviewers will help you add your contribution to the JDK.



Kind regards

Anthony




From: core-libs-dev  on behalf of 
Raffaello Giulietti 
Sent: Monday, May 6, 2019 2:08:48 PM
To: core-libs-dev
Subject: [PATCH] 4511638: Double.toString(double) sometimes produces incorrect 
results

Hi,

no new code this time, only a warm invitation to review the rather
substantial patch submitted on 2019-04-18 [1] and the CSR [2].

I spent an insane amount of (unpaid) free time offering my contribution
to solve a bug known since about 15 years. Thus, I think it is
understandable that I'd like to see my efforts come to a successful
conclusion, ideally for OpenJDK 13.


Greetings
Raffaello


P.S.
Some enjoyable properties of the novel algorithm:
* No objects are instantiated, except, of course, the resulting String.
* Loop-free core algorithm.
* Only int and long arithmetic.
* No divisions at all.
* 16x speedup (jmh).
* Randomized, yet reproducible deep diving tests (jtreg).
* Clear, unambiguous spec.
* All floats have been tested to fully meet the spec.
* Fully documented in [3] or in comments.



[1]
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059783.html
[2] https://bugs.openjdk.java.net/browse/JDK-8202555
[3] https://drive.google.com/open?id=1KLtG_LaIbK9ETXI290zqCxvBW94dj058

RE: ZipFileSystem performance regression

[Anthony Vanelverdinghe]

Hi



Alan already proposed the following RFEs when NIO2 was introduced: a memory 
FileSystemProvider [1], and a virtual FileSystemProvider [2]

With these, the javac use case could work as follows:

1) create a VirtualFileSystem, with all sources mounted under “/src”, a 
MemoryFileSystem mounted at “/target”, and a ZipFileSystem mounted at “/dist”

2) from then on, simply work with the VirtualFileSystem: read from /src, write 
compilation artifacts to /target, and finally assemble a .jar file by copying 
from /target to /dist



I think these providers would elegantly solve most use cases. And those 
providers could then be enhanced with further features as desired. For example, 
the memory FileSystem could allow on-the-fly compression, encryption, etc.



[1] https://bugs.openjdk.java.net/browse/JDK-8002315

[2] https://bugs.openjdk.java.net/browse/JDK-8002316



Kind regards

Anthony




From: core-libs-dev  on behalf of Peter 
Levart 
Sent: Wednesday, April 17, 2019 9:23:08 AM
To: Claes Redestad; Lance Andersen; Xueming Shen
Cc: core-libs-dev@openjdk.java.net
Subject: Re: ZipFileSystem performance regression

Just a thought...

Would it be feasible to create a brand new "Generic Caching Filesystem"
implementation that would delegate to another filesystem for persistent
storage (be it ZipFilesystem or any other) and implement interesting
caching strategies (lazy flushing, concurrent flushing, etc...)

So instead of parameterizing a concrete filesystem (e.g. ZipFilesystem),
the filesystems could be layered to achieve the desired performance
characteristics.

What do you think? Is the existing filesystem API suitable for such
layering?

Regards, Peter

On 4/16/19 11:50 PM, Claes Redestad wrote:
> It sounds reasonable to have that as an option, but I'd like to see it
> requested by some user first. And at least one (micro-)benchmark where
> keeping entries uncompressed in memory actually shows significant
> positive impact.
>
> I can see it might have the opposite effect depending on how often that
> memory is inflated/deflated and whether or not the inflated entries
> cause high enough memory pressure to make GC activity spike. javac might
> be one application where it could be negative as it's tuned to run with
> a rather small max heap and already spends significant time doing GCs.
>
> /Claes
>
> On 2019-04-16 23:20, Lance Andersen wrote:
>> Would it be worth  adding a ZIP File System property similar to
>> createNew which enables/disables the change that Claes has made
>> having the default be the pre-jdk 12 functionality?
>>
>>
>>> On Apr 16, 2019, at 4:50 PM, Xueming Shen 
>>> wrote:
>>>
>>> Well, have to admitted I didn't expect your use scenario when made
>>> the change. Thought as a
>>>
>>> filesystem runtime access performance has more weight compared to
>>> shutdown performance...
>>>
>>> basically you are no using zipfs as a filesystem, but another jar
>>> tool that happens to have
>>>
>>> better in/out concurrent performance. Yes, back then I was working
>>> on using zipfs as a memory
>>>
>>> filesystem. One possible usage is that javac to use it as its
>>> filesystem (temp?) to write out compiled
>>>
>>> class files ... so I thought I can have better performance if I can
>>> keep those classes uncompressed
>>>
>>> until the zip/jarfs is closed and written to a "jar" file.
>>>
>>> That said, regression is a regression, we probably want to get the
>>> performance back for your
>>>
>>> use scenario. Just wanted to give you guys some background what
>>> happened back then.
>>>
>>>
>>> -Sherman
>>>
>>>
>>> On 4/16/19 12:54 PM, Lennart Börjeson wrote:
 I’m using the tool I wrote to compress directories with thousands
 of log files. The standard zip utility (as well as my utility when
 run with JDK 12) takes up to an hour of user time to create the
 archive, on our server class 40+ core servers this is reduced to
 1–2 minutes.

 So while I understand the motivation for the change, I don’t get
 why you would want to use ZipFs for what in essence is a RAM disk,
 *unless* you want it compressed in memory?

 Oh well. Do we need a new option for this?

 /Lennart Börjeson

 Electrogramma ab iPhono meo missum est

> 16 apr. 2019 kl. 21:44 skrev Xueming Shen :
>
> One of the motivations back then is to speed up the performance of
> accessing
>
> those entries, means you don't have to deflate/inflate those
> new/updated entries
>
> during the lifetime of that zipfilesystem. Those updated entries
> only get compressed
>
> when go to storage. So the regression is more like a trade off of
> performance of
>
> different usages. (it also simplifies the logic on handing
> different types of entries ...)
>
>
> One idea I experimented long time ago for jartool is to
> concurrently write out
>
> entries when need compression ... it 

RE: RFR - JDK-8203442 String::transform (Code Review)

[Anthony Vanelverdinghe]

Hi Remi, Brian,



What about ‘asInputTo’ or some variant thereof (e.g. using Argument or the 
abbreviated Arg instead of Input; or dropping/replacing either of the 
prefix/suffix)?



This would give, for example:

`raw html`.asInputTo(HtmlDocument::new)

source.stream().asInputTo(this::maybeAddFilter)



To me, this name clearly relates the object with its argument: the object 
serves ‘as input to’ the argument (plus it hints at the fact that the argument 
is a Function). And since it’s just using general terminology, it’s applicable 
in a broad context, yet is unlikely to clash or look awkward anywhere.



Kind regards,

Anthony






From: core-libs-dev  on behalf of Remi 
Forax 
Sent: Wednesday, November 14, 2018 12:36:16 PM
To: Brian Goetz
Cc: core-libs-dev
Subject: Re: RFR - JDK-8203442 String::transform (Code Review)

Hi Brian,

- Mail original -
> De: "Brian Goetz" 
> À: "Peter Levart" 
> Cc: "core-libs-dev" 
> Envoyé: Mardi 13 Novembre 2018 15:37:31
> Objet: Re: RFR - JDK-8203442 String::transform (Code Review)

>> An argument against re-using the name map() for this String method is that
>> Stream.map() and Optional.map() act on the element(s) of the "container" the
>> method is invoked upon, and return the same raw part of type with type
>> parameter adjusted, while String.map() would be passing the target object
>> itself to the function and returning an arbitrary type.
>
> This is exactly why we did not choose the name map() when this feature was
> proposed.  (Such is life when a platform is accrete-only; you have to live 
> with
> your past decisions, good and bad.)
>
> (I didn’t see the proposal to rename transform() to map() fly by; I would have
> made the same comments as Peter.)

as i said earlier, we have https://bugs.openjdk.java.net/browse/JDK-8140283 
that asks for the same kind of method on Stream,
i think it's a good idea to have some kind or coordination here.
While 'transform' is a ok name for String, it's a bad name for Stream. All 
intermediary ops are transformations, but 'transform' doesn't subsume them. A 
method named transform will act as a magnet for Stream newbies that want to do 
transformation on a stream.

Perhaps 'chain' as proposed in JDK-8140283 is a better name ?

>
>> Other languages have introduced an operator to solve that issue (function 
>> call
>> syntax is not fluent) like by example the operator '|>' as in "foo" |>
>> Utils.capitalizeFirstLetter.
>
> Yes, we know :)  But we don’t have any current plans to do that, nor use-site
> extension methods, nor does it seem likely to come to the top of the language
> priority list very soon.  So its not a choice between |> and .transform(); 
> it’s
> a choice between transform() and nothing.  Picking transform() seems pretty
> good here.
>
> Stephen raised the issue of a “broader context”; indeed, the intention of 
> adding
> a transform() method here was that it would be feasible to add to other types
> for which it was suitable.  String is an obvious candidate for “do it first”,
> but this is within a broader context.

ok about the rationale, we just need to find a good name.

Rémi

RE: JDK-8124977: deadlock between engineers? (cmdline encoding challenges on Windows)

[Anthony Vanelverdinghe]

Thanks for the status update and insightful details, Sherman, they’re much 
appreciated.



Kind regards,

Anthony






From: core-libs-dev  on behalf of 
Xueming Shen 
Sent: Friday, September 14, 2018 12:38:28 AM
To: core-libs-dev@openjdk.java.net
Subject: Re: JDK-8124977: deadlock between engineers? (cmdline encoding 
challenges on Windows)

On 9/13/18, 3:36 PM, Xueming Shen wrote:
> Anthony,
>
> I don't see/recall there was any response to my last review comment
> [1]. The proposed
> change in webrev.05 [2], in which it forces the internal system
> property sun.jnu.encoding
> to be always "utf8", is incomplete and incorrect (see my comment [3]
> for why).

oops, there is one sentence is missing here :-

There are three system properties inside jvm to deal with this "native
encoding" issue:


>
> file.encoding: how to interpret, by default, the content in/from the
> outside of the vm, file, socket
>  for example
> sun.jnu.encoding:
>  how to talk to the platform APIs, on Windows
> platform for example, how to
>  to de/encoding the bytes to/from the Win32 "A"
> version of APIs, for example
>  CreateFileA(char* fname). It's also used to
> "interpret" the bytes in those
>  char* of the interface between jvm and libraries.
> sun.stdout/err.encoding:
>  how to talk to the "console" when you output to
> the std err/out when
>  the std in/out is linked the a real console/term
>
> So the bottom line is that you can't just simply override
> sun.jnu.encoding to utf8 on windows
> before we either migrate all our "A" version win32 system call to "W"
> (do autf8->utf16) or
> to put yet another layer there to convert "utf8->mb" before calling
> the "A" version win32.
>
> An alternative is to limit the scope of the problem, to only have some
> alternative/
> workaround solution for the arguments passed to Java main class. For
> example to do something
> in libjli/java_md.c/CreateApplicationArgs() when decoding the char* to
> jstring via
> LauncherHelper.makePaltformString(...), which uses sun.jnu.encoding
> now. (my guess is
> this is where the idea of overwriting sun.jnu.encoding comes from).
>
> Currently I don't think there is anyone from Oracle actively working
> on this issue. I'm not
> sure if engineer from Microsoft is still working on it.
>
> Thanks,
> Sherman
>
> [1]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/039070.html
> [2] http://cr.openjdk.java.net/~kshoop/8124977/webrev.05/
> [3]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/039068.html
>
> On 9/12/18, 10:29 AM, Anthony Vanelverdinghe wrote:
>> Hi
>>
>> What is the status of this issue [1]? It addresses some long-standing
>> issues with Java's Unicode support on Windows and was contributed by
>> a team of Microsoft engineers. However, it seems to have gone dormant
>> right before the finish line, and I can't really figure out who's
>> waiting for whom at this point.
>>
>> A little reconstruction from what I could find:
>> In January 2015, Martin Sawicki made the initial proposal to address
>> the cmdline encoding challenges on Windows [2]
>>  From June to September, there were ongoing discussions [3][4][5][6]
>> In November, this was shortly picked up again by the Oracle engineers
>> [7]
>> In January 2016, after a ping by a Microsoft engineer, the
>> discussions restarted [8]
>> In February 2016, the patch seemed nearly ready for integration, with
>> an Oracle engineer asking whom to mention as contributors etc. [9]
>> Since then, I was unable to find any messages related to this issue
>>
>> (Personally, I would love to see this issue progress, in order to be
>> able to associate Java programs with file extensions on Windows. This
>> is currently problematic, since a file containing Unicode characters
>> will not get passed correctly as an argument to the Java program)
>>
>> Kind regards,
>> Anthony
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8124977
>> [2]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-January/031068.html
>> [3]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/034226.html
>> [4]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-July/034488.html
>> [5]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-August/034838.html
>> [6]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035466.html
>> [7]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036769.html
>> [8]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/037927.html
>> [9]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/039013.html
>

JDK-8124977: deadlock between engineers? (cmdline encoding challenges on Windows)

[Anthony Vanelverdinghe]

Hi

What is the status of this issue [1]? It addresses some long-standing issues 
with Java's Unicode support on Windows and was contributed by a team of 
Microsoft engineers. However, it seems to have gone dormant right before the 
finish line, and I can't really figure out who's waiting for whom at this point.

A little reconstruction from what I could find:
In January 2015, Martin Sawicki made the initial proposal to address the 
cmdline encoding challenges on Windows [2]
>From June to September, there were ongoing discussions [3][4][5][6]
In November, this was shortly picked up again by the Oracle engineers [7]
In January 2016, after a ping by a Microsoft engineer, the discussions 
restarted [8]
In February 2016, the patch seemed nearly ready for integration, with an Oracle 
engineer asking whom to mention as contributors etc. [9]
Since then, I was unable to find any messages related to this issue

(Personally, I would love to see this issue progress, in order to be able to 
associate Java programs with file extensions on Windows. This is currently 
problematic, since a file containing Unicode characters will not get passed 
correctly as an argument to the Java program)

Kind regards,
Anthony

[1] https://bugs.openjdk.java.net/browse/JDK-8124977
[2] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-January/031068.html
[3] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/034226.html
[4] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-July/034488.html
[5] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-August/034838.html
[6] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035466.html
[7] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036769.html
[8] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/037927.html
[9] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/039013.html

Re: Optional.isEmpty()

[Anthony Vanelverdinghe]

Hi Peter

I'd say no: it's merely the negation of an existing method, and given 
that the bar for adding methods to Optional is set very high (see e.g. 
[1] and [2]), I don't see how this one would meet it.


Moreover, I don't see any issues with simply writing:

return !cf.findModule(target).isPresent();

and there are plenty of one-liners that don't use boolean negation as 
well, for example:


return cf.findModule(target).orElse(null) == null; // [3]
return cf.findModule(target).map(m -> false).orElse(true); // [4]
return cf.findModule(target).stream().count() == 0;
return cf.findModule(target).stream().allMatch(m -> false);
return cf.findModule(target).isPresent() ^ true;
...
return cf.findModule(target).equals(Optional.empty());

Adding a static import to the last one gets you very close to the 
proposed method already:

return cf.findModule(target).equals(empty());

Of course, another option is to introduce a variable to avoid the 
combination of boolean negation and method chaining:

boolean moduleFound = cf.findModule(target).isPresent();
return !moduleFound;

Kind regards, Anthony

[1] https://bugs.openjdk.java.net/browse/JDK-8057557
[2] https://bugs.openjdk.java.net/browse/JDK-8058707
[3] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2017-April/012273.html
[4] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2017-April/012290.html


On 22/04/2017 11:40, peter.levart at gmail.com (Peter Levart) wrote:

Hi,

Seeing the following line in some JDK test that was up for review:

  return cf.findModule(target).orElse(null) == null;

I immediately jumped to suggest it would look better if written as:

  return !cf.findModule(target).isPresent();


But then I leaned back and asked myself: "Would it really?"

The boolean negation in front of a chain of method calls tends to
visually disappear from the view when we finally reach the offending
method and might get missed when quickly browsing the code. In this
respect, ".orElse(null) == null" is actually better. But the best would
of course be something like that:

  return cf.findModule(target).isEmpty();

What do you think? Would this pull its weight?

Regards, Peter

Re: Shouldn't InputStream/Files::readAllBytes throw something other than OutOfMemoryError?

[Anthony Vanelverdinghe]

Hi Chris

Point well taken, but being unable to create a native thread is 
definitely a VirtualMachineError, and personally I don't care whether 
the JVM throws an OOME or any other kind of VirtualMachineError in that 
case.


My point was that I don't see how being unable to return a result 
because of a language limitation (i.e. arrays being indexed by integers, 
and thus limited to 2G for byte[]), has anything to do with 
OutOfMemoryError. I believe it would be much more logical to throw a 
recoverable RuntimeException in this case (e.g. 
java.lang.ArrayOverflowException, as an analog of 
java.nio.BufferOverflowException).


Anthony

On 12/03/2017 15:53, Christoph Engelbert wrote:

Hey Anthony,

The meaning is already overloaded, as "Cannot create native thread"
is also an OutOfMemoryError and in like 99% of the cases means
"Linux ran out of filehandles". The chance the OS really couldn't
allocate a thread for the reason of no main memory available is very
narrow :)

Chris

Am 3/12/2017 um 3:24 PM schrieb Anthony Vanelverdinghe:

Files::readAllBytes is specified to throw an OutOfMemoryError "if
an array of the required size cannot be allocated, for example the
file is larger that 2G". Now in Java 9, InputStream::readAllBytes
does the same.

However, this overloads the meaning of OutOfMemoryError: either
"the JVM is out of memory" or "the resultant array would require
long-based indices".

In my opinion, this overloading is problematic, because:
- OutOfMemoryError has very clear semantics, and I don't see the
link between OOME and the fact that a resultant byte[] would need
to be >2G. If I have 5G of free heap space, and try to read a 3G
file, I'd expect something like an UnsupportedOperationException,
but definitely not an OutOfMemoryError.
- the former meaning is an actual Error, whereas the latter is an
Exception from which the application can recover.
- developers might be tempted to catch the OOME and retry to read
the file/input stream in chunks, no matter the cause of the OOME.

What was the rationale for using OutOfMemory here? And would it
still be possible to change this before Rampdown Phase 2?

Kind regards,
Anthony

Shouldn't InputStream/Files::readAllBytes throw something other than OutOfMemoryError?

[Anthony Vanelverdinghe]

Files::readAllBytes is specified to throw an OutOfMemoryError "if an 
array of the required size cannot be allocated, for example the file is 
larger that 2G". Now in Java 9, InputStream::readAllBytes does the same.


However, this overloads the meaning of OutOfMemoryError: either "the JVM 
is out of memory" or "the resultant array would require long-based indices".


In my opinion, this overloading is problematic, because:
- OutOfMemoryError has very clear semantics, and I don't see the link 
between OOME and the fact that a resultant byte[] would need to be >2G. 
If I have 5G of free heap space, and try to read a 3G file, I'd expect 
something like an UnsupportedOperationException, but definitely not an 
OutOfMemoryError.
- the former meaning is an actual Error, whereas the latter is an 
Exception from which the application can recover.
- developers might be tempted to catch the OOME and retry to read the 
file/input stream in chunks, no matter the cause of the OOME.


What was the rationale for using OutOfMemory here? And would it still be 
possible to change this before Rampdown Phase 2?


Kind regards,
Anthony

Re: PING: RFR: JDK-4347142: Need method to set Password protection to Zip entries

[Anthony Vanelverdinghe]

Hi Yuji, Sherman et al.

PKWARE's "Strong Encryption Specification" [1] indeed warns about 
patents, but how/why does this affect Winzip's AE-1 and AE-2 [2]?


I don't mind if decryption support is added for the "Traditional 
Encryption". However, I believe it would be wrong to introduce 
encryption support for a known-to-be-broken encryption method in the 
JDK. Using the argument of "it's good enough for our case", I could also 
argue that Base64 qualifies as an encryption method, or that SSLv2 is an 
appropriate choice to secure network connections.


However, if this is to be added, it should be supported by the zipfs 
FileSystemProvider as well. The passphrase handler could be passed by a 
property in the environment, e.g. Function which 
would provide the passphrase for a given path, or Optional.empty() if 
the Path is unencrypted.


Kind regards, Anthony

[1] https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT
[2] http://www.winzip.com/win/en/aes_info.htm

On 8/04/2016 13:04, KUBOTA Yuji wrote:

Hi Sherman and all,

Thank you for all comments about this proposal!

Thank Tomoyuki and Stephen for sharing your needs.

Thank Bernd for sharing your information. However, I am
afraid that AE1 and AE2 may have a risk of patent issue,
while "Traditional PKWare encryption" is explicitly free
from the risk according to the PKWare documents.
Therefore, I think this proposed implementation to support
legacy zip specification is meaningful as Tomoyuki said.

We got some positive opinions / needs. I am glad if you
consider the proposal acceptable for JDK.
If this proposed implementation is acceptable, I and
Yasumasa will improve it by corresponding to comments
which given by Sherman and Stephen.

Thanks!
Yuji

2016-03-29 6:41 GMT+09:00 Xueming Shen :

Yasumasa

It's a tricky call. To be honest, as I said at the very beginning, I'm not
sure whether
or not it's a good idea and worth the efffort to push this into the j.u.zip
package to
support the "traditional PKWare encryption", which is known to be "seriously
flawed,
and in particular is vulnerable to known-plaintext attacks" (from wiki),
while I fully
understood it is not a concern in your "problem-free" use scenario. Just
wonder
if there is anyone else on the list that has/had the need for such
encryption feature
in the past. It would be preferred to have more input (agree, disagree) to
make the
final decision.

Here are some comments regarding the proposed implementation,

(1) The changes in native code are probably no longer needed. The native
path is
  left there for vm directly access.
(2) I'm concerned about the footprint increase for the ZipEntry class (the
proposed
  change is adding 3 more fields into the entry class). Given this is
something rarely
  needed/used, and we might have hundreds and thousands of zip/jar
entries during
  startup/runtime, it might be desired to avoid adding these fields into
ZipEntry class.
  A possible alternative to have a dedicated entry class extended from
the ZipEntry
  to hold those extra fields and methods, EncryptionZipEntry for example.
So the
  proposed encryption support code will not have any impact to the
existing use of
  ZipInput/OutputStream/File.
(3) I'm also not comfortable to add the "encryption engine" logic into the
in/deflater
  stream classes, while it might be convenient to do so from
implementation point
  of view. Given the encryption is something between the raw bites in the
zip file
  and the in/deflating layer, it might need an extra layer there, though
I'm not sure
  if it's easy to do so.

The webrev below is what I think might be better for the ZipFile and
ZipOutputStream
to have an extra layer in between to handle the encryption. Not try to test
if it really
works, just throw in the idea for discussion.

http://cr.openjdk.java.net/~sherman/zencrypt/webrev/

No, I did not try the ZIS, the "pushback" stream there might be a little
tricky:-)

-Sherman


On 03/28/2016 02:34 AM, Yasumasa Suenaga wrote:

PING: Could you review it?
We want to move forward this enhancement.

Thanks,

Yasumasa
2016/03/19 22:01 "Yasumasa Suenaga":


Hi Alan,


I think the main issue here is to decide whether the API
should be extended for encryption or not.

We've discussed on the premise that we add the API for supporting ZIP
encryption.
In this context, Sherman tried to implement AES encryption extending
current API. [1]

IMHO, ZIP encryption should be implemented as extention of current ZIP
API
because encryption/decryption will process for each ZIP entries.

According to Developers' Guide, guideline for adding new API is TBD. [2]
What should I do next?


Thanks,

Yasumsa


[1]

http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/037903.html
[2] http://openjdk.java.net/guide/changePlanning.html#api


On 2016/03/19 0:25, Alan Bateman wrote:

On 18/03/2016 15:02, Yasumasa Suenaga wrote:

Hi 

Re: RFR 8124977 cmdline encoding challenges on Windows

[Anthony Vanelverdinghe]

Hi

Thanks for taking on these challenges.

I would like to be able to associate file types with a Java program [1]. 
Currently, the Java program doesn't receive the argument (i.e. the path 
to the file that was double-clicked) correctly if it contains Unicode 
characters. I assume this would be possible by specifying 
-Dwindows.UnicodeConsole=true in the ftype command string, right?


I would also like for System.out and System.err to use the specified 
Unicode charset. Currently, System.out and System.err use Windows-1252 
(on my system), so they can't readily be used for Unicode output. A 
quick look at the webrev suggests that this hasn't been addressed yet.


Kind regards,
Anthony

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-May/033132.html



On 22/06/2015 23:01, Kirk Shoop (MS OPEN TECH) wrote:

Hi,

Issue:
   https://bugs.openjdk.java.net/browse/JDK-8124977

Webrev:
   http://cr.openjdk.java.net/~kshoop/8124977/

This webrev intends to address interaction between Windows console and java 
apps.

Two switches were added that change the behavior of the launcher. The defaults 
do not change the launcher behavior.

   -Dwindows.UnicodeConsole=true - switches on Unicode support in the Windows 
console. This optional switch causes the launcher to call GetCommandLineW() and 
parse the arguments in unicode. It also modifies how the codepage for console 
output is selected.

   -Dfile.encoding.unicode=UTF-8 - identifies Unicode charset to use; If not 
specified, UTF-8 is used by default. Ignored when windows.UnicodeConsole is not set to 
true. When the first switch is used, this optional switch allows the codepage for console 
output to be controlled.

I would like to get feedback on the approach here and any additional work that 
is required solve these particular Unicode issues on Windows.

Kirk

Unicode command-line parameters on Windows

[Anthony Vanelverdinghe]

Hi

I would like to use a Java program for Windows file associations. 
However, this doesn't work when the file to be opened contains non-ASCII 
Unicode characters in its path.


There are several related issues about Windows Unicode support (e.g. 
JDK-4488646, JDK-4519026, JDK-4900150, JDK-6475368, JDK-6937897, 
JDK-8029584), some of which are resolved with Future Project and the 
last one having Fix Version 10 [1].
A while ago there was also a draft JEP about this with ID 8047097 [2]. 
However, the JEP is no longer available  the associated JDK issue is 
private.
In January a code submission was proposed by Microsoft developers [3], 
but it's unclear from the thread what happened with the submission.
From these observations, I'd guess there will be a Windows Unicode 
support project targeted for Java SE 10?


Who can shed some light on the current plans for this? Will there be 
improvements in this area in Java SE 9?


Thanks in advance,
Anthony

[1] https://bugs.openjdk.java.net/browse/JDK-8029584
[2] http://openjdk.java.net/jeps/8047097
[3] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-January/031068.html

Re: RFR 9: 8077350 Process API Updates Implementation Review

[Anthony Vanelverdinghe]

Thanks for your clarifications Roger. I'm very much in favor of your 
suggestion for naming the method supportsNormalTermination.


Kind regards,
Anthony


On 11/04/2015 20:35, Roger Riggs wrote:

Hi Anthony,

Thanks for the review and comments.

On 4/11/2015 5:00 AM, Anthony Vanelverdinghe wrote:

Hi Roger

In my opinion, the method supportsDestroyForcibly is unintuitive, 
for the following 2 reasons:


- it's named supportsXxx, where Xxx is the name of a method in the 
same class. So as a user of this API, I would intuitively assume that 
supportsDestroyForcibly is related to destroyForcibly, and means 
whether or not destroyForcibly actually forcibly terminates the 
process.


- supports has a positive connotation. However, if 
supportsDestroyForcibly returns true, this means that the 
implementation of destroy is forcible. And destroyForcibly either 
invokes destroy (default implementation) or is overridden with a 
compliant implementation. So in other words: if 
supportsDestroyForcibly returns true, it's impossible to allow the 
process to shut down cleanly. This, in my opinion, is a bad thing, so 
the method indicating this behavior should not start with supports.
The naming of the method has been problematic; as is the ambiguous 
semantics of Process.destroy

which are historical.


Therefore I would like to propose to replace 
supportsDestroyForcibly with supportsDestroy, which returns the 
negation of what's currently returned by supportsDestroyForcibly.
I'm not sure this is clearer.  Both destroy() and destroyForcibly() 
always support the destruction
of the process and without reading more closely, supportDestroy() 
would always be true.


Perhaps supportNormalTermination() would reflect the more general 
understanding of the behavior

and the spec could provide the details in relation to the destroy method.



Another question I have is: what happens if destroy or 
destroyForcibly are called on processes other than the current 
process  its children, i.e. a process that is in allProcesses but 
isn't current   isn't in allChildren (e.g. the parent process of 
the current process)?

The current spec does not guarantee that the process will be destroyed;
processes can protect themselves against the signals (SIGTERM or SIGKILL)
just as it does not make any assertion about the timing of the process 
state change.


The behavior is to attempt to kill the process but it may not work for 
a variety of

reasons, not all of which can be detected or reported to the caller.
For ProcessHandle.destroy, if the OS does not allow the signal delivered,
the destory/destroyForcibly method could return a boolean.
Alternatively, Martin's request to throw arbitrary signals would be a 
new method

with more os specific behavior and exceptions.

The decoupling of ProcessHandle and Process opens the possibility
to have different semantics than for Process.destroy/destroyForcibly.

Roger


Kind regards,
Anthony


On 9/04/2015 22:00, Roger Riggs wrote:

Please review the API and implementation of the Process API Updates
described inJEP 102 
https://bugs.openjdk.java.net/browse/JDK-8046092. Please review 
and comment by April 23rd.


The recommendation to make ProcessHandle an interface is included
allowing the new functions to be extended by Process subclasses.
The implementation covers all functions on Unix, Windows, Solaris, 
and Mac OS X.


The API doc: http://cr.openjdk.java.net/~rriggs/ph-apidraft/

The webrev: http://cr.openjdk.java.net/~rriggs/webrev-ph

Issue: JDK-8077350 
https://bugs.openjdk.java.net/browse/JDK-8077350 Process API 
Updates Implementation


The code is in the jdk9 sandbox on branch JDK-8046092-branch.

Please review and comment, Roger

Re: RFR 9: 8077350 Process API Updates Implementation Review

[Anthony Vanelverdinghe]

Hi Roger

In my opinion, the method supportsDestroyForcibly is unintuitive, for 
the following 2 reasons:


- it's named supportsXxx, where Xxx is the name of a method in the 
same class. So as a user of this API, I would intuitively assume that 
supportsDestroyForcibly is related to destroyForcibly, and means 
whether or not destroyForcibly actually forcibly terminates the process.


- supports has a positive connotation. However, if 
supportsDestroyForcibly returns true, this means that the 
implementation of destroy is forcible. And destroyForcibly either 
invokes destroy (default implementation) or is overridden with a 
compliant implementation. So in other words: if 
supportsDestroyForcibly returns true, it's impossible to allow the 
process to shut down cleanly. This, in my opinion, is a bad thing, so 
the method indicating this behavior should not start with supports.


Therefore I would like to propose to replace supportsDestroyForcibly 
with supportsDestroy, which returns the negation of what's currently 
returned by supportsDestroyForcibly.


Another question I have is: what happens if destroy or 
destroyForcibly are called on processes other than the current process 
 its children, i.e. a process that is in allProcesses but isn't 
current   isn't in allChildren (e.g. the parent process of the 
current process)?


Kind regards,
Anthony


On 9/04/2015 22:00, Roger Riggs wrote:

Please review the API and implementation of the Process API Updates
described inJEP 102 
https://bugs.openjdk.java.net/browse/JDK-8046092. Please review and 
comment by April 23rd.


The recommendation to make ProcessHandle an interface is included
allowing the new functions to be extended by Process subclasses.
The implementation covers all functions on Unix, Windows, Solaris, and 
Mac OS X.


The API doc: http://cr.openjdk.java.net/~rriggs/ph-apidraft/

The webrev: http://cr.openjdk.java.net/~rriggs/webrev-ph

Issue: JDK-8077350 https://bugs.openjdk.java.net/browse/JDK-8077350 
Process API Updates Implementation


The code is in the jdk9 sandbox on branch JDK-8046092-branch.

Please review and comment, Roger

Re: JEP 102 Process Updates revised API draft

[Anthony Vanelverdinghe]

Hi Roger

This looks great already. My feedback is about process destruction:

Why isn't ProcessHandle::isDestroyForcible a static method?

For ProcessHandle::destroy and Process::destroy, I'd like to propose 
replacing
Whether the process represented by this Process object is forcibly 
terminated or not is implementation dependent.

with:
The process represented by this Process object is forcibly terminated 
if and only if isDestroyForcible returns true.


Process::destroyForcibly contains the following phrase: The default 
implementation of this method invokes destroy() and so may not forcibly 
terminate the process.
Why doesn't the default implementation throw 
UnsupportedOperationException if forcible termination is not supported 
on/implemented for the current platform? If I write code like: 
process.destroyForcibly().waitFor(), I'd assume it would finish in a 
timely manner, but due to the default implementation, this may actually 
not finish at all.


Kind regards, Anthony


On 10/02/2015 0:25, Roger Riggs wrote:

Hi,

After a protracted absence from working on JEP 102, the updated API draft
provides access to process hierarchies and individual process 
information;
as permitted by the OS. The relationship between Process and 
ProcessHandle

is clarified and the security model validated.

Both Processes and ProcessHandles can be monitored using 
CompletableFuture

for termination and to trigger additional actions on Process exit.
Information about processes includes the total cputime, starttime, user,
executable, and arguments.

Please review and comment:
   http://cr.openjdk.java.net/~rriggs/ph-apidraft/

Thanks, Roger