Re: [External] : Re: ReversibleCollection proposal

[Stuart Marks]

The line of discussion here was introduced by Remi, who was making an argument of 
the form "introducing a type cannot solve this particular problem, therefore, 
introducing a new type is not useful at all." I was providing an example of where 
the new type is useful as a method parameter. That's all.



Have you considered the alternative of a collection providing a Reversed view 
of itself, in the same sense that unmodifiable collections are views of an 
underlying collection?


The proposal does define ReversibleCollection::reversed as providing a reversed 
view, through which modifications to the underlying collection are visible, and to 
which modifications are written through to the underlying collection. Or are you 
talking about something different?


s'marks

On 4/30/21 4:15 PM, Alan Snyder wrote:

It sounds like the items processing maintainer would be looking for 
OrderedCollection and might or might not find ReversibleCollection. :-)

I suspect you would agree that OrderedCollection by itself is too weak to 
justify being a type. It’s basically Iterable with the extra bit that the 
iteration order is not an implementation artifact.

In this example, using the type system to detect a bug like the old bug seems 
like overkill. Even if the parameter were Ordered, it still might not be the 
*right* order. The maintainer of the client code needs to understand that.

Suppose the items processor wants to require that the parameter collection not 
contain duplicates. Would you suggest adding a type for that? Clearly Set would 
be just as unnecessarily restrictive as List was for ordering. Absurdity 
approaches…

The issue of Reversible remains, above and beyond Ordered. Have you considered 
the alternative of a collection providing a Reversed view of itself, in the 
same sense that unmodifiable collections are views of an underlying collection?

   Alan




On Apr 30, 2021, at 3:42 PM, Stuart Marks  wrote:

Consider the case of a large application or other system, one that's large 
enough to have lots of internal APIs, but that is built as a single unit, so 
release-to-release compatibility isn't an issue. Suppose there is some method

processItemsInOrder(List items)

that has to process items in the order in which they occur, because processing 
of later items might depend the processing of earlier ones. The maintainer of 
this API chose to accept a List as a parameter, because it's a common interface 
and it's clearly an ordered collection.

Now consider a client that gets items from different places, keeping them in 
order, but removing duplicates. It might do something like this:

var items = new LinkedHashSet();
items.addAll(getItemsFromSomeplace());
items.addAll(getItemsFromAnotherPlace());
items.addAll(getItemsFromSomeplaceElse());
processItemsInOrder(new ArrayList<>(items));

It turns out the copying of the items into an ArrayList is a performance 
bottleneck, so the maintainer of the client code asks the maintainer of the 
items processing code to change the API to accept Collection instead.

The items processing maintainer demurs, recalling that the API *did* accept 
Collection in the past, and a bug where somebody accidentally passed a HashSet 
resulted in a customer escalation because of item processing irregularities. In 
the aftermath of that escalation, the API was changed to List. The client 
maintainer reluctantly pursues alternatives for generating a deduplicated List.

But wait! Those Java guys added a ReversibleCollection interface in Java N. It 
has the desired property of being ordered, and conveniently it's a supertype of 
both List and LinkedHashSet. The items processing maintainer adjusts the API to 
consume ReversibleCollection, and the client maintainer removes the temporary 
ArrayList, and everybody is happy.

s'marks

Re: RFR: 8265356: need code example for getting canonical constructor of a Record [v2]

[Stuart Marks]

On Sat, 24 Apr 2021 01:32:45 GMT, Tagir F. Valeev  wrote:

>> I decided to show a complete static method in the example, so it could be 
>> copied to user utility class as is. Not sure if it's reasonable to add 
>> `assert cls.isRecord();` there. Also I don't know whether there's a 
>> limitation on max characters in the sample code. Probable a line break in 
>> `static \nConstructor getCanonicalConstructor(Class 
>> cls)` is unnecessary.
>> 
>> ---
>> Aside from this PR, I've found a couple of things to clean up in 
>> `java.lang.Class`:
>> 1. There's erroneous JavaDoc link in `getSimpleName()` JavaDoc (introduced 
>> by @jddarcy in #3038). It should be `#isArray()` instead of `isArray()`.
>> 2. Methods Atomic::casAnnotationType and Atomic::casAnnotationData have 
>> unused type parameters ``.
>> 3. Probably too much but AnnotationData can be nicely converted to a record! 
>> Not sure, probably nobody wants to have `java.lang.Record` initialized too 
>> early or increasing the footprint of such a basic class in the metaspace, so 
>> I don't insist on this.
>> 
>> 
>> private record AnnotationData(
>> Map, Annotation> annotations,
>> Map, Annotation> declaredAnnotations,
>> // Value of classRedefinedCount when we created this AnnotationData 
>> instance
>> int redefinedCount) {
>> }
>> 
>> 
>> Please tell me if it's ok to fix 1 and 2 along with this PR.
>
> Tagir F. Valeev has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Trailing space removed
>  - Add a reference from java.lang.Record to related Class methods
>  - Fix cosmetic issues

Overall looks fine modulo adding `@apiNote`. Since the changes are all either 
editorial/markup or informational, I don't think this needs a CSR. Well, the 
removal of an unused type variable strictly constitutes a signature change, but 
those methods aren't public APIs so I still think it's ok without a CSR.

src/java.base/share/classes/java/lang/Class.java line 2363:

> 2361:  * Conversely, if {@link #isRecord()} returns {@code true}, then 
> this method
> 2362:  * returns a non-null value.
> 2363:  *

I forgot to mention, this example should be within an `@apiNote`.

-

Marked as reviewed by smarks (Reviewer).

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

Re: ReversibleCollection proposal

[Stuart Marks]

You did not really answer to the real question, why should i use 
ReversibleCollection instead of a Collection as parameter.
You said that it's a better type because you can not send a HashSet, but as i 
said, sending a HashSet of 0 or 1 element is perfectly valid.
For me, we are not far from, it's typechecking for the sake of typechecking.


I thought I did answer it, but it might have been in response to a different message 
on a different part of the thread. But I'll make the case in a more explicit fashion 
here.


First, a couple background points related to this:

 - ReversibleCollection is not intended to, and indeed cannot represent all ordered 
collections. Queue is ordered and is not a ReversibleCollection, and there are 
likely other collections out there that are ordered but that implement Collection 
directly. Also, they might or might not be reversible.


 - I expect that few, if any Java SE APIs will be adjusted to use 
ReversibleCollection as a parameter type. Any APIs that use Collection but require 
an ordered type cannot be changed because of compatibility reasons.


Despite both of these points, I believe ReversibleCollection can be successful, and 
it can be useful in APIs as a parameter type. (The proposal itself uses 
ReversibleCollection and ReversibleSet as method return types.)


Consider the case of a large application or other system, one that's large enough to 
have lots of internal APIs, but that is built as a single unit, so 
release-to-release compatibility isn't an issue. Suppose there is some method


processItemsInOrder(List items)

that has to process items in the order in which they occur, because processing of 
later items might depend the processing of earlier ones. The maintainer of this API 
chose to accept a List as a parameter, because it's a common interface and it's 
clearly an ordered collection.


Now consider a client that gets items from different places, keeping them in order, 
but removing duplicates. It might do something like this:


var items = new LinkedHashSet();
items.addAll(getItemsFromSomeplace());
items.addAll(getItemsFromAnotherPlace());
items.addAll(getItemsFromSomeplaceElse());
processItemsInOrder(new ArrayList<>(items));

It turns out the copying of the items into an ArrayList is a performance bottleneck, 
so the maintainer of the client code asks the maintainer of the items processing 
code to change the API to accept Collection instead.


The items processing maintainer demurs, recalling that the API *did* accept 
Collection in the past, and a bug where somebody accidentally passed a HashSet 
resulted in a customer escalation because of item processing irregularities. In the 
aftermath of that escalation, the API was changed to List. The client maintainer 
reluctantly pursues alternatives for generating a deduplicated List.


But wait! Those Java guys added a ReversibleCollection interface in Java N. It has 
the desired property of being ordered, and conveniently it's a supertype of both 
List and LinkedHashSet. The items processing maintainer adjusts the API to consume 
ReversibleCollection, and the client maintainer removes the temporary ArrayList, and 
everybody is happy.


s'marks

Re: [External] : Re: ReversibleCollection proposal

[Stuart Marks]

OK, I think we can wrap up this portion of the thread.

As the proposal stands, it has add{First,Last} returning void instead of some useful 
value. For SortedSet and for LinkedHashMap's views, these throw UOE. Can we do better?


Collection has add(), Deque has add{First,Last} and offer{First,Last}, and 
BlockingDeque has put{First,Last}. I'm loathe to add new insertion methods that 
differ from these, either in signatures or semantics.


The BlockingDeque putX methods have blocking behavior, which only makes sense for a 
concurrent collection. Still, because these exist, we mustn't add putX methods 
elsewhere that have different semantics.


After having thought about this for a couple days, I think it's a really bad idea to 
reuse offerX methods. These would allow a collection to refuse the addition of an 
element and merely return a boolean indicating that. I can easily see people writing 
offerX code that doesn't check the return value, and if things shift around in their 
program, elements can be silently dropped. This would set a new precedent, as the 
present behavior is that collections can reject adding an element only by throwing 
an exception. Allowing refusal by returning 'false' would be a bad precedent.


So I think we're stuck with void-returning add{First,Last} methods.

Regarding throwing UOE, I think it's useful to distinguish between the LinkedHashMap 
views and SortedSet. Today, it's already the case that Map's view collections don't 
support addition, so the ReversibleX views of LinkedHashMap are similar in this regard.


SortedSet is different, as it's a top-level collection interface instead of a view 
collection. It's unusual for it not to support the addX operations. We explored some 
alternatives, such as throwing exceptions if preconditions aren't met. These seem 
fairly rare, and the alternative behaviors don't seem all that useful. In any case 
nothing emerged that was clearly better than simple UOE-throwing behavior.


OK, I think it's been useful to analyze various alternatives -- in particular I 
hadn't seriously considered the offerX methods -- but I'll leave the proposal as it 
stands regarding add{First,Last} methods.


s'marks



On 4/28/21 6:54 AM, Peter Levart wrote:


On 4/28/21 7:19 AM, Stuart Marks wrote:



On 4/27/21 9:17 AM, Anthony Vanelverdinghe wrote:

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."


Yes, to be clear, it was Stephen Coleborne who raised this previously [1] and it's 
my response that's quoted above.


Some further thoughts on this.

This is an example where, depending on the current state of the collection, the 
method might throw or it might succeed. This is useful in concurrent collections 
(such as the capacity-restricted Deque above), where the caller cannot check 
preconditions beforehand, because they might be out of date by the time the 
operation is attempted. In such cases the caller might not want to block, but 
instead it might catch the exception and report an error to its caller (or drop 
the request). Thus, calling the exception-throwing method is appropriate.


Something like SortedSet::addLast seems different, though. The state is the 
*values* of the elements already in the collection. This is something that can 
easily be checked, and probably should be checked beforehand:


    if (sortedSet.isEmpty() || sortedSet.last().compareTo(e) <= 0)
    sortedSet.add(e);
    else
    // e wouldn't be the last element, do something different



I was thinking more of a case where the else branch would actually throw 
IllegalStateException and do nothing else - a kind of add with precondition check. A 
precondition in the sense of Objects.requireNonNull(). I can't currently think of a 
real usecase now, so this kind of operation is probably very rare. Probably not 
useful since if you're adding to SortedSet, the order of elements added should not 
matter because SortedSet will sort them. If you just want to check the order of 
elements added 

Collection::getAny discussion

[Stuart Marks]

Hi Henri,

I've changed the subject of this thread because I think it's out of scope of the 
ReversibleCollection proposal. I don't mean to say that we can't talk about it, but 
I would like it to be decoupled from ReversibleCollection. I'm somewhat arbitrarily 
calling it "Collection::getAny" because something similar to that was mentioned by 
both Remi and Peter elsewhere in this thread. There is also a bunch of history in 
the bug database that contains related ideas.


Before we dive in, I want to explain why this is separate from ReversibleCollection. 
Most of the ideas, including yours, involve an implementation that does something 
like `iterator().next()`, in other words, getting the "first" element from an 
Iterator. Hey, there's getFirst() in ReversibleCollection, let's use that! No. The 
"first" element of an iterator is in general an arbitrary element, which is 
different from the "first" element in the structural ordering of elements provided 
by a ReversibleCollection. The "arbitrary" notion is captured by "getAny" so that's 
what I'll use as a working term. (Of course any actual API we might add would have a 
better name if we can find one.)


For a historical perspective, let's dig into the bug database and take a look at 
this bug:


https://bugs.openjdk.java.net/browse/JDK-4939317

This requests a method Collection.get(Object). This searches the collection for an 
element that equals() the argument and returns the element, or it returns null if 
the element isn't found. (Recall in those days it was impossible to add a method to 
an interface without breaking compatibility, so it also proposes various workarounds 
that are no longer necessary.)


There's a comment from Josh Bloch saying that Collection should have had a get() 
method as well as a no-arg remove() method. Well ok then! And he points to the 
then-new Queue interface that was delivered in Java SE 5. Queue adds the following 
methods that seem relevant to this discussion:


 * E element() -- gets the head element, throws NSEE if empty
 * E remove() -- removes and returns the head element, throws NSEE if empty

(It also adds peek() and poll(), which are similar to the above except they return 
null if empty.)


This is kind of odd, in that none of these methods satisfy what the bug's submitter 
was requesting, which is a one-arg get() method. Also, these methods are on Queue, 
which doesn't really help with collections in general.


You're asking for something that's somewhat different, which you called the "find 
the first element when there is only one" problem. Here, there's a precondition that 
the collection have a single element. (It's not clear to me what should happen if 
the collection has zero or more than one element.)


To throw a couple more variations into the mix, Guava has a couple Collectors (for 
streams) that do interesting things. The class is MoreCollectors:


https://guava.dev/releases/30.1.1-jre/api/docs/com/google/common/collect/MoreCollectors.html

and the collectors are:

 * onlyElement -- if source has 1 element, returns it; throws NSEE if empty, IAE 
if > 1
 * toOptional -- if source has 0 or 1 elements, returns an Optional; otherwise 
throws

These apply to streams, but I think you can see the applicability to Collection as 
well. In particular, your proposal is similar to what onlyElement would look like if 
it were on Collection.


Let's summarize the variations so far:

 * preconditions: exactly one element, at-most-one, at-least-one
 * behavior if preconditions not met: return null, return empty Optional, throw
   exception
 * remove element or just peek
 * match a particular element, or return an arbitrary element

That's a lot of variations!

Before we talk about specific APIs, though, I wanted to talk about use cases. Which 
of these variations are more useful or less useful? Which are likely to appear in 
code? Henri gave a fairly specific example with a reasonable "success" case 
(preconditions met) but it's not clear what should happen if the preconditions 
aren't met. Clearly an API would have to choose. What would the use site look like? 
In particular, does the use site always have to check for null, or catch an 
exception, or something?


Answers to these questions will help determine what APIs, if any, we might want 
to add.

***

There's another thing looming in the distance, which is pattern matching. You might 
have seen one of Brian Goetz's talks on pattern matching futures. You can get a 
glimpse of some upcoming pattern matching features in JEP 405.


    http://openjdk.java.net/jeps/405

In particular, this JEP extends pattern matching with an /extraction/ step, where, 
if there is a match, record or array components can be extracted and bound to local 
variables. This is a step closer to /deconstruction patterns/, where arbitrary 
classes and interfaces (not just records or arrays) can participate in pattern 
matching. (See discussion of this at the end of the JEP.)

Re: 8252827: Caching Integer.toString just like Integer.valueOf

[Stuart Marks]

(Catching up on old email threads.)

I don't have much to add to the technical discussion here. Yes, caching the Integer 
instances seems obsolescent, and it seems unlikely that caching String conversions 
will be helpful. I've gone ahead and closed out the bug. [1]


On triaging bugs... we do triage bugs pretty effectively, I think. However, 
enhancement requests (like this one) tend to languish. People tend to notice ones 
that are obviously bad ideas or obviously good ideas and act on them quickly. 
However, there are a lot of items that are neither obviously good nor bad, so they 
just sit there. Sometimes they just sit there even after there has been some email 
discussion on them. :-)


Sometimes I think we take these requests a bit too seriously. It looks to me like 
the submitter put about five minutes of thought into the request, and we've 
collectively probably spent 10x that dealing with it. I probably put too much effort 
into this bug myself; instead of researching the history, I could have simply closed 
it with a comment saying, "Closed, per discussion in " which 
would have been reasonable.


Anyway, it's closed.

s'marks


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


On 4/17/21 5:19 AM, Raffaello Giulietti wrote:

Hi,

in view of Integer becoming a primitive class [1], the IntegerCache is probably 
going to disappear.


For a small, fixed range like the one you are proposing [-1, 16], there's no real 
need for a separate cache class. You could have a switch in the implementation of 
toString(), with the string literals then being part of the constant pool of the 
class. Not free beer, but supported by the VM since day 0.


It's only when the range is open that you'd need a cache similar to 
IntegerCache.


My 2 cents as well :-)
Raffaello



[1] https://openjdk.java.net/jeps/402

On 2021-04-17 11:18, Laurent Bourgès wrote:

Hi,

I read the JBS bug and I interpret it as:
- IntegerCache provides Integer instances for [-128, 127] by default
- Having Integer.toString(int) could behave the same or at least cache most 
probable values like [-1 to 16] or using the IntegerCache range.


It looks trivial and potentially could benefits to jdk itself to reduce memory 
garbage : is Integer.toString(int) widely used in the jdk codebase ?


Finally it can be alternatively implemented in application's code.

My 2 cents,
Laurent

Le sam. 17 avr. 2021 à 11:06, Raffaello Giulietti > a écrit :




    On 2021-04-17 07:07, David Holmes wrote:
 > On 17/04/2021 4:54 am, Raffaello Giulietti wrote:
 >> I guess the reporter meant to limit the cache range similarly to
    the
 >> one used for valueOf().
 >>
 >> I have no clue about the benefit/cost ratio for the proposed String
 >> cache. It really depends on usage, workload, etc. One can easily
 >> imagine both extreme scenarios but it's hard to tell how the
    average
 >> one would look.
 >>
 >> My post is only about either solving the issue by implementing the
 >> cache, which I can contribute to; or closing it because of lack of
 >> real-world need or interest.
 >
 > Caching for the sake of caching is not an objective in itself.
    Unless
 > the caching can be shown to solve a real problem, and the
    strategy for
 > managing the cache is well-defined, then I would just close the
 > enhancement request. (Historically whether an issue we don't have
    any
 > firm plans to address is just left open "forever" or closed, depends
 > very much on who does the bug triaging in that area. :) )
 >
 > Cheers,
 > David
 >


    Indeed, the impression is that many of the issues are probably open
    because it's unclear whether they should be addressed with some
    implementation or spec effort or whether they should be closed.
    Triaging
    is certainly a harder job than it appears at first sight ;-)

    It would be useful to have a kind of "suspended" or "limbo" resolution
    state on the JBS for issues like this one, so people searching for more
    compelling open ones would not encounter them.

    Personally, I would close this issue without a "fix"; or "suspend" it.


    Greetings
    Raffaello



 >>
 >> Greetings
 >> Raffaello
 >>
 >>
 >> On 2021-04-16 20:36, Roger Riggs wrote:
 >>> Hi,
 >>>
 >>> Is there any way to quantify the savings?
 >>> And what technique can be applied to limit the size of the cache.
 >>> The size of the small integer cache is somewhat arbitrary.
 >>>
 >>> Regards, Roger
 >>>
 >>> On 4/16/21 12:48 PM, Raffaello Giulietti wrote:
  Hello,
 
  does the enhancement proposed in [1] make sense, both today
    and when
  wrappers will be migrated to primitive classes?
  If so, it should be rather simple to add it and I could
    prepare a PR.
  If not, the issue might perhaps be closed.
  

Re: [External] : Re: ReversibleCollection proposal

[Stuart Marks]

On 4/27/21 9:17 AM, Anthony Vanelverdinghe wrote:

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."


Yes, to be clear, it was Stephen Coleborne who raised this previously [1] and it's 
my response that's quoted above.


Some further thoughts on this.

This is an example where, depending on the current state of the collection, the 
method might throw or it might succeed. This is useful in concurrent collections 
(such as the capacity-restricted Deque above), where the caller cannot check 
preconditions beforehand, because they might be out of date by the time the 
operation is attempted. In such cases the caller might not want to block, but 
instead it might catch the exception and report an error to its caller (or drop the 
request). Thus, calling the exception-throwing method is appropriate.


Something like SortedSet::addLast seems different, though. The state is the *values* 
of the elements already in the collection. This is something that can easily be 
checked, and probably should be checked beforehand:


if (sortedSet.isEmpty() || sortedSet.last().compareTo(e) <= 0)
sortedSet.add(e);
else
// e wouldn't be the last element, do something different

Now this is a fair bit of code, and it would be shorter just to call 
sortedSet.addLast(e). But does that actually help? What if e is already in the set 
and is the last element? Is catching an exception really what we want to do if e 
wouldn't be the last element? Maybe we'd want to do nothing instead. If so, catching 
an exception in order to do nothing is extra work.


Again, I'd like to hear about use cases for a conditionally-throwing version of 
addLast et. al. I don't want to be limited by failure of imagination, but it does 
seem like this kind of behavior would be useful only in a narrow set of cases where 
it happens to do exactly the right thing. Otherwise, it just gets in the way, and 
its behavior is pretty obscure. So, color me skeptical.



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


[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2021-April/076505.html


- 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.


Right, the current proposal copies the addX method signatures from Deque into 
ReversibleCollection.


I think it was Mike Duigou who said that a method that returns void is a missed 
opportunity. The idea is, the library probably has some useful bit of information it 
can return. If the caller doesn't need it, the caller can just ignore it.


On Collection::add returning a boolean, most calls to this ignore the return value, 
and the boolean "did this collection change as a result of this call?" is only 
occasionally useful. Sometimes it can be used for little tricks, such as an easy way 
to determine if a stream contains all unique elements:


stream.allMatch(new HashSet<>()::add)

But really, the boolean return value doesn't seem all that useful.

Still, it could be added. The methods couldn't be called addFirst/addLast as Anthony 
pointed out, as this would clash with those methods already on Deque. However, Deque 
already contains boolean-returning 

Re: ReversibleCollection proposal

[Stuart Marks]

On 4/27/21 2:25 AM, Peter Levart wrote:
Right, I'm persuaded. Now if only the problems of transition to the usage of new 
type that Remi is worried about were mild enough. Source (in)compatibility is not 
that important if workarounds exist that are also backwards source compatible so 
that the same codebase can be compiled with different JDKs. Binary compatibility is 
important. And I guess there is a variant of the proposal that includes 
ReversibleCollection and ReversibleSet and is binary compatible.


Yes, the source incompatibilities with the types seem to involve type inference 
(e.g., use of var choosing differently based on new types in the library) so it 
should be possible to make minor source code changes to adjust or override the type 
that's inferred.


On binary compatibility, that comes mostly from the newly introduced methods 
(reversed, addFirst etc.) and from adding covariant overrides to LinkedHashSet. I 
guess the "variant" you mention is adding new methods with the new return types 
(e.g., reversibleEntrySet) instead of covariant overrides. Or was it something else?



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.


- 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.


Anthony Vanelverdinghe replied to both of these points so I'll follow up in a reply 
to his message.


- Perhaps unrelated to this proposal, but just for completeness, existing Collection 
interface could be extended with methods like: getAny() and removeAny().


Yes, I think there's something going on here, but the issues lead off in a different 
direction, which I think would be a distraction from ReversibleCollection. So I'd 
like to keep this as a separate topic. (But it's not a prohibition against talking 
about them.) Indeed, I see that Henri Tremblay has coincidentally (?) raised a 
similar topic, so I'll reply further to his message.


s'marks

Re: ReversibleCollection proposal

[Stuart Marks]

On 4/25/21 11:09 AM, Peter Levart wrote:
When making this proposition, I was only thinking of how to enable new 
yet-nonexistent operations on collections that have order / are reversible and that 
the code calling these methods would already knows that it is dealing with ordered / 
reversible collections. I wasn't thinking about how to prevent mistakes like passing 
unordered collection to code that expects ordered / reversible collection. This can 
only be solved in two ways. The way immutability of collections is solved (throw 
exception in runtime when requesting operation that makes no sense in unordered 
collection) or by introducing new type - ReversibleCollection that solves this in 
compile time. So I take back my vote for suggestion to introduce methods like 
getFirst(), removeFirst() that would return arbitrary element. There's still a 
possibility for those methods to throw at runtime though. But I don't know whether 
this would be so nicely accepted as immutability was. WDYT?


Consider some collection implementation X that has some semantics and a suite of 
operations that support those semantics. An implementation can throw 
UnsupportedOperationException if for some reason that operation doesn't make sense 
for the implementation, but fundamentally that implementation still has "X-ness".


Look at Arrays.asList for example. It's backed by an array, so add and remove 
operations aren't supported. But it's still fundamentally a List in that it has N 
elements, is ordered, the elements are numbered from 0 to N-1, sublists can be 
obtained between two arbitrary indexes, its ListIterator supports iteration in both 
directions, it has a clear and useful definition for equals() and hashCode(), etc.


Similarly, an unmodifiable List is still a List.

Now consider adding {add,get,remove}{First,Last} methods to the Collection 
interface. Collection doesn't have any semantics for these methods itself, so we're 
hoping that there is some sensible mapping from these methods to appropriate 
operations on Collections implementations or subinterfaces.


 * Presumably for unordered collections like HashSet, all of these methods would be 
left to throw UOE.


 * A queue has a head and a tail, so perhaps these are mapped to first and last, 
respectively, and Queue would support addLast and removeFirst but not addFirst and 
removeLast. But that wouldn't work for PriorityQueue.


 * A stack has a "top": the most recently pushed element. Does the stack top 
correspond to the first or the last? It could be either -- in fact, in the JDK, for 
Stack the top is the last element but for Deque the top is the first element.


 * For some other ordered collection, who knows what a reasonable mapping would 
be?

This approach feels backwards to me. Instead of starting from an abstraction that 
has strong semantics and then applying all (or at least most) of the applicable 
methods, we're starting with a suite of methods and looking around for relationships 
to a variety of abstractions that might or might not have much in common. That seems 
like a recipe for a bad API to me.


s'marks

Re: ReversibleCollection proposal

[Stuart Marks]

An API can not use ReversibleCollection as parameter before all the 
implementations of Collection that have an ordering are updated to implement 
ReversibleCollection instead of Collection.

By example, if I add a method
   public void foo(ReversibleCollection c)
in my library, people that want to be able to use foo with an existing 
collection with an ordering have to wait that collection implementation to be 
updated.

So i will not add a method like foo in my API until enough (whatever enough 
means) implementation of Collection with an ordering have migrated.

This is a similar issue as the migration from Python 2 to Python 3, it takes at 
least 10 years, so in 10 years, I will be able to had a public method that 
takes a ReversibleCollection in my API,
without people not be able to using it.


This is a gigantic overstatement of the issue.

It is true that an API *in the JDK* as a practical matter cannot change a Collection 
parameter to ReversibleCollection, because it will break any callers that are 
currently passing Collection. The same is true of any library APIs where 
compatibility is a concern.


However, it can be quite useful in many contexts to use ReversibleCollection as a 
parameter. Newly introduced APIs can use ReversibleCollection. Certain APIs that 
consume List could reasonably be adjusted to consume a ReversibleCollection instead. 
(More likely an overload would probably be added in order to preserve binary 
compatibility.) In contexts where compatibility across maintenance boundaries is not 
an issue -- that is, where APIs can be changed incompatibly by updating all callers 
-- it's easy to imagine a variety of scenarios where a List or Collection parameter 
can usefully be adjusted to ReversibleCollection.



ReversibleCollection unifies a broad set of existing collections without
requiring
any retrofitting at all. Many applications and libraries don't have their own
collection implementations; they just use the ones in the JDK. They can benefit
from
the new APIs in ReversibleCollection immediately, without the need for any
retrofitting at all.


nope, many applications do not use *only* the collection from the JDK,
but also collections that comes from other libraries than msut to be upgraded 
to use ReversibleCollection.


We're not actually disagreeing. Many applications and libraries *do* use only the 
collections in the JDK. It is probably also true that other collection libraries are 
popular and are *also* used in many places.


Non-JDK collections libraries benefit, because any List, SortedSet, and Deque 
implementations provided by these libraries are automatically retrofitted to 
implement ReversibleCollection.



ReversibleCollection can be a solution, it's also not the best way to say that 
you want a collection with an ordering,
any collections with zero or one element is also a valid ordered collection, so 
by example a HashSet with only one element is a valid argument.


Sure, this is vacuously true. It doesn't seem to be an argument against 
ReversibleCollection.



If we can ascertain (via code searching) that introducing covariant overrides to
LinkedHashMap introduces minimal incompatibilities, we might decide to go ahead
with
the change. (What's considered "minimal" is of course up for discussion.)

If however we decide the incompatibilities are too great, a fallback plan would
be
to introduce new methods reversibleEntrySet() et al for the reversible views.
This
would be a little bit disappointing, but it doesn't invalidate the
ReversibleCollection/ReversibleSet concept.


It shows that the concept ReversibleCollection/ReversibleSet concept while 
useful out of the blue, needs to be shoehorned in the JDK, making it not worth 
it because the cost vs reward has changed.


Ah, now you're talking about cost vs. benefit. Thus, you must agree that 
ReversibleCollection/ReversibleSet is a valid concept!


It's premature to make a decision based on costs and benefits -- really, risks of 
incompatibilities. I've done some preliminary searching and what I've found so far 
is promising; the risk of incompatibilities seems low. Additional searching might 
reveal information that changes the situation.


s'marks

Re: ReversibleCollection proposal

[Stuart Marks]

On 4/22/21 8:36 AM, Stephen Colebourne wrote:

On Thu, 22 Apr 2021 at 13:58, Remi Forax  wrote:

I would like to preserve the invariant that, when calling a method on a 
Collection/iterator, an UnsupportedOperationException only occurs when trying 
to mutate that collection.
If we are ok with that, this means that addFirst can not be a default method of 
Collection.


This implementation meets the invariant:

  public interface Collection  {
default void addFirst(E e) { add(e); }
default E getFirst() { return iterator().next(); }
default E removeFirst() {
  var i = iterator(); i.next();
  i.remove();
}
  }

This is what I intended anyway, ie that its OK for "first" to work on
an unordered collection, just that addFirst() has very weak semantics
wrt first-ness.

"Ensures that this collection contains the specified element(optional
operation). This has the same basic behaviour as add(E), but
implementations may choose to add the element in first position if
they have some kind of guarantee of ordering. An exception should not
be thrown unless add(E) would also have thrown the exception."


What seems to be going on here is that people want to add generality by promoting 
this set of methods from ReversibleCollection up to Collection. Unfortunately, the 
only way to do so is to make the specification so loose that the semantics are 
destroyed.


Having these methods on Collection could lead to a situation where calling addFirst 
and then getFirst might result in getFirst returning a different element from what 
was passed to addFirst. This doesn't make any sense for methods that have "first" in 
the name.


It seems like there's some other use case that people have in mind, such as "get an 
arbitrary element from this collection". The obvious way to implement this is 
iterator().next() which gets the "first" element from the iteration and so people 
are latching onto the name "getFirst". But let's not confuse the "first" element in 
a semantic ordering with the "first" element returned by an iterator. They're distinct.


s'marks

Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v6]

[Stuart Marks]

On Wed, 21 Apr 2021 13:13:16 GMT, Jim Laskey  wrote:

>> Move makeXXXSpilterator from public (@hidden) to protected. No API ch
>
> Jim Laskey has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains eight additional commits since 
> the last revision:
> 
>  - Merge branch 'master' into 8265137
>  - Remove @hidden
>  - Correct the hierarchy of Random
>  - Remove extraneous references to makeXXXSpliterator
>  - Move makeXXXSpliterator methods to RandomSupport
>  - change static final from 'proxy' to 'PROXY'
>  - Make makeXXXSpliterator final
>  - Move makeXXXSpilterator from public (@hidden) to protected. No API change.

Overall looks good.

AbstractSpliteratorGenerator now is a bit weird because it's a utility class 
that has static methods, _and_ it's an abstract superclass that has instance 
methods that mostly just call the static methods. Seems to me that additional 
cleanup is possible. It might not be worth it right now, since the main point 
of this PR -- to remove "leakage" of these helper methods into the public API 
-- has been achieved.

The RandomXXXSpliterator nested classes could also stand some scrutiny. The 
constructors consume a RandomGenerator, which is stored in an instance 
variable. Various comments still refer to this as an 
AbstractSpliteratorGenerator. (See line 961 for example; can't comment directly 
because it's not part of this commit.)

But it's not clear to me that the RandomXXXSpliterator classes really need a 
full RandomGenerator. For example, as far as I can see, RandomIntSpliterator 
_only_ needs `nextInt` to generate int values; therefore it could be 
`IntSupplier`. Similar for long and double. (I'm applying the Interface 
Segregation Principle here.) But again this is probably a cleanup for another 
day.

src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 
1433:

> 1431: private AbstractSpliteratorGenerator() {
> 1432: }
> 1433: 

This comment is wrong now since there are instances of this class (really, 
subclasses). The constructor should probably simply be removed.

-

Marked as reviewed by smarks (Reviewer).

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

Re: [External] : Re: ReversibleCollection proposal

[Stuart Marks]

On 4/19/21 2:01 PM, Remi Forax wrote:

- Mail original -
Thinking a little bit about your proposal,
introducing an interface right in the middle of a hierarchy is not a backward 
compatible change
(you have an issue when the compiler has to use the lowest upper bound).

By example
   void m(List> list) { ... }

   var list = List.of(new LinkedHashSet(), List.of("foo"));
   m(list);  // does not compile anymore

currently the type of list is List> but with your proposal, the type 
will be List>


Yes, interesting. Not too difficult to fix though. Either change the method 
declaration to


void m(List> list)

or change the 'var' to an explicit declaration of List>.

s'marks

Re: ReversibleCollection proposal

[Stuart Marks]

On 4/19/21 4:05 PM, fo...@univ-mlv.fr wrote:

  * There's a useful clump of semantics here, combined with sensible operations
  that
rely on those semantics. There are a lot of places in the spec where there is
hedging of the form "if the collection has an ordering, then... otherwise the
results are undefined". This is unnecessary in the context of a new type.
Furthermore, the new operations fit well with the new types' semantics, with no
hedging necessary.


You can only say that a reversible collection has an ordering. But usually you 
want the opposite, all collections that have an ordering are a reversible 
collection. But while this can be true for the implementations inside the JDK 
that will never be true in Java because there all already collection 
implementations with an ordering which do not implement ReversibleCollection.

Sadly, this ship has sailed.
The spec will still have to say "if the collection has an ordering", otherwise 
the new semantics is not a backward compatible.


The value being provided here is that the ReversibleCollection interface provides a 
context within which specs no longer need to hedge about "if the collection has an 
ordering". APIs that produce or consume ReversibleCollection no longer need to 
include this hedge, or have disclaimers about ordering. Potential new 
ReversibleCollection implementations also need not worry about this issue in the 
same way they did when they were forced to implement Collection directly.


Of course there will always be ordered collections that don't implement 
ReversibleCollection. (Examples of this are the Queue-but-not-Deque implementations 
in the JDK and 3rd party ordered collections that implement Collection directly.) 
Existing APIs that produce or consume Collection but have stipulations about 
ordering may need to remain, although some could be adjusted depending on the context.


You rightly point out that this problem can never be solved in general. However, 
it's not a goal for ReversibleCollection to represent *all* ordered collections, so 
it's hardly a criticism that it doesn't solve a problem that cannot be solved.



  * These semantics appear across a broad range of existing collection types and
implementations. It's quite valuable to have a type that unifies the common
pieces
of List, Deque, SortedSet, and LinkedHashSet into a single abstraction.


yes in term of documentation, but in Java, it also means that you can use that 
interface as a type.

For a List, a Deque or a Set, there are known algorithms that takes such type 
has parameter so it makes sense to have such type.
For a ReversibleCollection, i do not see many codes that will benefit taking a 
ReversibleCollection as parameter instead of using Collection.


It seems likely that many public APIs (both in the JDK and in external libraries) 
will continue to use Collection, for compatibility reasons.


In certain cases (such as LinkedHashMap, see below) the APIs could be adjusted, if 
the compatibility impact is small or can be mitigated.


ReversibleCollection unifies a broad set of existing collections without requiring 
any retrofitting at all. Many applications and libraries don't have their own 
collection implementations; they just use the ones in the JDK. They can benefit from 
the new APIs in ReversibleCollection immediately, without the need for any 
retrofitting at all.


ReversibleCollection also provide opportunities for new code and for existing APIs 
that don't have stringent compatibility constraints. Consider an application that 
has a method wants to consume an ordered collection; a reasonable API would take a 
List as a parameter.


Suppose a caller happened to have a LinkedHashSet in the right order (for example, 
because it wanted to deduplicate the elements). Either the caller would be forced to 
copy the LHS into a List before passing it, or the callee could adjust its parameter 
to be Collection -- but this creates the possibility of bugs if a HashSet is passed 
by accident. Using ReversibleCollection as a parameter type fixes this problem.



  * It's useful to have a new type for return types in APIs. Consider
LinkedHashMap's entrySet, keySet, and values views. While we clearly get by with
having these return Set or Collection today, we need a place to put the new
methods.
Either they go on Collection (and have extremely weak semantics) or we define
new
interfaces.


Even if you define a new interface, you can not change the return type of 
LinkedHashMap entrySet because it's not a backward compatible change. 
LinkedHashMap is not final so you have to update all subclasses of 
LinkedHashMap too.


As I've mentioned, introducing the covariant overrides for LinkedHashMap views is a 
compatibility *risk* but by itself this is not dispositive.


In the past we had no way to assess this risk, so we simply avoided ever making such 
changes. This might be why NavigableMap introduced navigableKeySet to return a 
NavigableSet instead of 

Re: RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes [v2]

[Stuart Marks]

On Mon, 19 Apr 2021 22:12:30 GMT, Ian Graves  wrote:

>> Clarifying note on comments mode to explicitly note that whitespace within 
>> character classes is ignored.
>
> Ian Graves has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adding differences to Perl 5 note

A few minor wording adjustments. Please update the CSR accordingly and I'll 
review it too.

src/java.base/share/classes/java/util/regex/Pattern.java line 762:

> 760:  *character classes. In this class, whitespace inside of character 
> classes
> 761:  *must be escaped to be considered as part of the regular expression 
> when in
> 762:  *comments mode.  

Editorial: the run of italicized words makes this a bit hard to follow. Suggest:

In Perl, free-spacing mode (which is called comments mode in 
this class)

src/java.base/share/classes/java/util/regex/Pattern.java line 832:

> 830:  *  Note that comments mode ignores whitespace within a character 
> class
> 831:  * contained in a pattern string. Such whitespace needs to be escaped
> 832:  * in order to be treated as if comments mode were not enabled. 

I think this is good, but 1) it would probably be better placed in the "In this 
mode" paragraph above, around line 825; and 2) it's normative so it shouldn't 
say "Note that" (which makes it sound informative).

I'd also reword the second sentence a bit, something like

Such whitespace needs to be escaped in order to be considered significant.

-

Changes requested by smarks (Reviewer).

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

Re: RFR: 8262744: Formatter '%g' conversion uses wrong format for BigDecimal rounding up to limits [v3]

[Stuart Marks]

On Fri, 16 Apr 2021 16:08:53 GMT, Ian Graves  wrote:

>> This fixes a bug where the formatting code for `%g` flags incorrectly tries 
>> to round `BigDecimal` after determining whether it should be a decimal 
>> numeric format or a scientific numeric format. The solution rounds before 
>> determining the correct format.
>
> Ian Graves has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Inlining some single use variables

src/java.base/share/classes/java/util/Formatter.java line 3827:

> 3825: if ((value.equals(BigDecimal.ZERO))
> 3826: || ((value.compareTo(BigDecimal.valueOf(1, 4)) != 
> -1)
> 3827: && (value.compareTo(BigDecimal.valueOf(1, 
> -prec)) == -1))) {

Note that `compareTo` in general specifies a negative, zero, or positive return 
value, but BigDecimal and BigInteger specify a return value of -1, 0, and 1. So 
the code here that compares against -1 is strictly correct. However, the 
BigDecimal/BigInteger.compareTo docs say "The suggested idiom..." is a relative 
comparison against zero.

Indeed, the BigDecimal::compareTo method does always seem to return -1, 0, or 1 
so this code is not incorrect. Well, maybe. I checked quickly and the 
BigDecimal comparison logic is fairly intricate (and also runs through 
BigInteger) so I might have missed something. Also, BigDecimal is subclassable, 
so an override of `compareTo` might return something other than -1, 0, or 1, 
even though strictly speaking this would violate the BigDecimal spec.

I'm wondering if there should be a followup bug that changes these tests to `>= 
0` and `< 0`.

-

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

Re: RFR: 8265356: need code example for getting canonical constructor of a Record

[Stuart Marks]

On Sat, 17 Apr 2021 08:55:59 GMT, Tagir F. Valeev  wrote:

> I decided to show a complete static method in the example, so it could be 
> copied to user utility class as is. Not sure if it's reasonable to add 
> `assert cls.isRecord();` there. Also I don't know whether there's a 
> limitation on max characters in the sample code. Probable a line break in 
> `static \nConstructor getCanonicalConstructor(Class 
> cls)` is unnecessary.
> 
> ---
> Aside from this PR, I've found a couple of things to clean up in 
> `java.lang.Class`:
> 1. There's erroneous JavaDoc link in `getSimpleName()` JavaDoc (introduced by 
> @jddarcy in #3038). It should be `#isArray()` instead of `isArray()`.
> 2. Methods Atomic::casAnnotationType and Atomic::casAnnotationData have 
> unused type parameters ``.
> 3. Probably too much but AnnotationData can be nicely converted to a record! 
> Not sure, probably nobody wants to have `java.lang.Record` initialized too 
> early or increasing the footprint of such a basic class in the metaspace, so 
> I don't insist on this.
> 
> 
> private record AnnotationData(
> Map, Annotation> annotations,
> Map, Annotation> declaredAnnotations,
> // Value of classRedefinedCount when we created this AnnotationData 
> instance
> int redefinedCount) {
> }
> 
> 
> Please tell me if it's ok to fix 1 and 2 along with this PR.

Thanks for writing this example.

I think that the example lines can be longer. I'd suggest putting the main part 
of the method declaration on the same line as `static `, but 
leaving the `throws` clause on the next line.

I think including the small cleanups (1) and (2) in this PR is fine. Changing 
`AnnotationData` to be a record seems like it might have other effects, so I'd 
leave that one out.

One other thing I'd like to see is a link to this example code from places 
where people are likely to look for it. The class doc for `java.lang.Record` 
has a definition for "canonical constructor" so it would be nice to link to the 
example here. Something like "For further information about how to find the 
canonical constructor reflectively, see Class::getRecordComponents." (With 
appropriate javadoc markup.) This could either be a parenthetical comment 
somewhere in the "canonical constructor" discussion, or possibly a separate 
paragraph in the `@apiNote` section below.

-

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

Re: ReversibleCollection proposal

[Stuart Marks]

On 4/17/21 9:49 AM, Remi Forax wrote:

I think the analysis is spot on but I don't think the proposed solution is the 
right one.

Introducing a new interface (two in this case) has a really huge cost and in 
this case, i've trouble to see why i will want to write a method that takes a 
ReversibleCollection as parameter, ReversibleCollection as a type does not seem 
to be useful. About the cost of introducing a new collection interface, it 
means that we also have to update all emptyXXX, singleXXX, uncheckedXXX with a 
new interface, and that only for the JDK. Every libraries that proxy 
collections has also to be updated to take care of this new type, otherwise the 
integration with an application that uses this new type and the library is not 
seamless anymore.


Hi Remi,

Thanks for looking at this. There are several issues intertwined here, so let's try 
to break it down.


There are a bunch of factory methods emptyX, singletonX, etc. that give instances of 
particular types. As Tagir noted, List is a ReversibleCollection, so you can just 
use emptyList or List.of or whatever if you need an instance of 
ReversibleCollection. Also, SortedSet/NavigableSet are ReversibleSets, so if you 
need an empty ReversibleSet, you can use Collections.emptyNavigableSet. There's no 
singletonNavigableSet, but you could do


Collections.unmodifiableNavigableSet(new TreeSet<>(List.of(x)))

which is a bit of a mouthful, but it's possible. (The fact that there's no 
Collections.singletonNavigableSet may mean this use case is so narrow that few 
people really needed it.)


As for the wrappers, synchronizedX and checkedX are mostly disused, so I don't see a 
need to provide those wrappers. Having unmodifiable RC and RS is probably useful, so 
I could see adding those. That's a certain amount of work, but not a tremendous cost.



All these methods can be introduced has default methods on the existing 
collection interfaces, there is no need of a special interface (or two) for 
that.


Clearly it's possible to get by without new interfaces. After all the collections 
framework has gotten by without them for 20+ years already. It's certainly possible 
to fill out the individual interfaces (List, Deque, Sorted/NavSet) and classes 
(LinkedHashSet, LinkedHashMap) with methods that are all similar to each other, and 
that would be useful. Or as Peter Levart pointed out, we could push all of them up 
to Collection and write the specs in a loose enough way to encompass even unordered 
collections.


Any of those alternatives (do nothing, add individual methods, add loosely-spec'd 
methods to Collection) are *possible* to do. However, I think you know how minimal 
we are with the JDK APIs, and we wouldn't have proposed new interfaces without good 
cause. Thus, I think introducing new *types* here is useful, for the following reasons.


 * There's a useful clump of semantics here, combined with sensible operations that 
rely on those semantics. There are a lot of places in the spec where there is 
hedging of the form "if the collection has an ordering, then... otherwise the 
results are undefined". This is unnecessary in the context of a new type. 
Furthermore, the new operations fit well with the new types' semantics, with no 
hedging necessary.


 * These semantics appear across a broad range of existing collection types and 
implementations. It's quite valuable to have a type that unifies the common pieces 
of List, Deque, SortedSet, and LinkedHashSet into a single abstraction.


 * It's useful to have a new type for parameters in APIs. There are times when one 
wants to accept something like a List -or- a LinkedHashSet. Typically one would 
accept a Collection and then write a spec with hedging as above ("the argument 
collection must have a defined ordering"). But this also allows bugs, for example if 
somebody accidentally passes a HashSet. Having ReversibleCollection helps this problem.


 * It's useful to have a new type for return types in APIs. Consider 
LinkedHashMap's entrySet, keySet, and values views. While we clearly get by with 
having these return Set or Collection today, we need a place to put the new methods. 
Either they go on Collection (and have extremely weak semantics) or we define new 
interfaces.


 * It's a useful interface for new collection implementations. There are data 
structures that are ordered, double-ended, and reversible, but for which 
implementing List is too onerous. Supporting int indexes in various List APIs is 
where stumbling blocks usually occur. I note that the LinkedHashMap view collections 
are examples of this that already exist in the prototype code. (Other possibilities 
might be something like SpinedBuffer or chunked linked lists.)


In general, I agree that there should be strict criteria for introducing new 
interfaces into collections, but I think this proposal meets them.


s'marks

Re: ReversibleCollection proposal

[Stuart Marks]

On 4/17/21 4:49 AM, Tagir Valeev wrote:

Great proposal, thanks!

It has most functionality from my previous proposal, except the
ability to iterate LinkedHashSet starting from a specific element.
Well, probably we can skip this for now.


Thanks! And thanks for making that proposal a year ago; the ideas there and in the 
ensuing discussion clearly informed this proposal.


I did spend a bunch of time thinking about the "start from a specific element" case. 
Certainly it isn't difficult to create an iterator that starts from a specific 
element and proceeds in either direction. However, it's 2021, and nobody wants to 
program using Iterators anymore!! :-)


Perhaps an obvious extension is a LinkedHashSet subset-view from a particular 
element to the end (or beginning). This is kind of interesting, however, it leads 
off into the weeds. That is, the complexity in doing this seemed to outweigh the 
rest of the proposal, especially for the limited value it supplies, so I've left it 
aside for now.


It's possible to get a stream of LHS elements starting or ending at a particular 
element though a combination of reversed, takeWhile, and dropWhile.


s'marks

Re: ReversibleCollection proposal

[Stuart Marks]

On 4/17/21 2:48 AM, 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.


Great!


# 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.


I probably could have explained this better in the writeup.

"Reversible" is a stronger concept (i.e., it implies more things) than "Ordered". A 
reversible collection is ordered, it is traversable in both directions, both ends 
are accessible, and it affords operations at both ends. However, being ordered does 
not imply reversibility. For example, a PriorityQueue is ordered, but you can't get 
to the tail or iterate in reverse without a bunch of computation.



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.


This is certainly an unpleasant asymmetry. But this is the Collections Framework; we 
should be used to unpleasant asymmetries. :-)


My typical examples of this are: various Map views like entrySet are that elements 
can be removed but they cannot be added; elements in Arrays.asList can be set but 
not added or removed; CopyOnWriteArrayList is mutable, but its iterators are not; etc.


Of course the presence of asymmetries doesn't justify the addition of more. But 
there is a precedent here: collections will implement an interface if they can 
support "most" of the operations, and the ones that aren't appropriate will throw 
UnsupportedOperationException. That's the precedent I'm following here with 
SortedSet (and NavigableSet).


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.



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


Deque defines a full set of operations at both ends, including:

{ add, get, remove } x { First, Last }

so it seemed sensible to take that complete set and promote it to 
ReversibleCollection.

s'marks

Re: ReversibleCollection proposal

[Stuart Marks]

On 4/16/21 3:06 PM, Donald Raab wrote:

We should be cautious when adding new APIs to existing interfaces. There may be 
libraries which extend JDK types and already have reversed(), toReversed() or 
asReversed() APIs and corresponding interfaces.

There are OrderedIterable and ReversibleIterable interfaces and asReversed() 
and toReversed() methods in the Eclipse Collections API.


Hi Don, thanks for looking at the proposal.

Certainly a lot of care is required when introducing new interfaces, new methods on 
existing interfaces, and covariant overrides. I believe the primary risks are with 
name clashes and with return type mismatches.


On name clashes, "reversed" seems to thread the needle well, as I cannot find any 
methods with that exact name on Eclipse Collections, Guava, Apache Commons 
Collections, or several others. There are methods with similar names, of course, 
such as "reverse", "asReversed", and "toReversed" but they shouldn't cause name 
conflicts.


(There might be semantic conflicts, such as creating an reversed copy instead of a 
reversed view, but I don't think that can be helped.)


If there are no name clashes with "reversed", the covariant overrides in the 
sub-interfaces won't a problem. The covariant overrides on existing methods (such as 
LinkedHashMap.entrySet) are a greater danger, I think. There are a lot of 
LinkedHashMap subclasses (several dozen are visible in the Yawkat browser) but only 
one had a conflicting override. It's trivially fixable, but nonetheless it's an 
incompatibility.


I'm also concerned about conflicts over the other method names; something like 
addFirst() is a pretty obvious method to add to a custom List implementation. I 
haven't seen any, but that doesn't mean there aren't any.


s'marks

ReversibleCollection proposal

[Stuart Marks]

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 = list.listIterator(list.size()); it.hasPrevious(); ) {
var e = it.previous();
...
}

Streaming the elements of a List in reverse order is even worse. One approach would 
be to implement a reverse-ordered Iterator that wraps a ListIterator and delegates 
hasNext/next calls to the ListIterator's hasPrevious/previous methods. Then, this 
Iterator can be turned into a Spliterator, which is then turned into a Stream. (The 
code to do this is left as an exercise for the reader.) Obtaining the elements in 
reverse via other means -- forEach() or toArray() -- is similarly difficult.


Obtaining the elements in reverse order can be accomplished by adopting a concept 
from 

Re: RFR: 8264148: Update spec for exceptions retrofitted for exception chaining

[Stuart Marks]

On Wed, 24 Mar 2021 23:17:46 GMT, Joe Darcy  wrote:

> 8264148: Update spec for exceptions retrofitted for exception chaining

Marked as reviewed by smarks (Reviewer).

-

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

Re: RFR: 8264148: Update spec for exceptions retrofitted for exception chaining

[Stuart Marks]

On Wed, 24 Mar 2021 23:17:46 GMT, Joe Darcy  wrote:

> 8264148: Update spec for exceptions retrofitted for exception chaining

The removal of the obsolescent "As of release 1.4, this exception has been 
retrofitted..." is good. Changing the calls from the other exception-getting 
methods to `getCause()` is also good. I'm less sure of the utility of 
deprecating these older methods. The deprecation will issue warning messages; 
is there any benefit to the calling code to migrating from the older methods to 
`getCause()`? If they're exactly equivalent, then I think the benefits are 
small, compared to the cost of dealing with warnings. Thus for most of these 
cases I think that not deprecating the older methods is reasonable, and perhaps 
the explanation should be converted to an `@apiNote`.

(The considerations for the JDK itself are different, though, which is why I 
support changing the call sites.)

One special case is the **public field** in `WriteAbortedException`. This is 
really bad and something ought to be done about this, including deprecation, 
and maybe more. This implies that the exception is mutable, right? Hrrmph. 
Isn't there a general rule that once the cause has been set (either via a 
constructor or via initCause) the exception is immutable? Maybe the field 
should be deprecated, and `getCause()` should return the cause from the 
superclass. That's a behavior change of course, and I don't know how to assess 
the compatibility impact. But the current situation just seems wrong.

-

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

Re: RFR: 8263102: Expand documention of Method.isBridge [v5]

[Stuart Marks]

On Tue, 9 Mar 2021 23:38:26 GMT, Joe Darcy  wrote:

>> The existing documentation of Method.isBridge isn't terribly helpful to the 
>> reader. This RFE proposes to given a common example of how bridge methods 
>> are used. The JLS does *not* have a section discussing bridge methods in 
>> detail; bridge methods are a compilation technique for lowering the Java 
>> language to the JVM, they are not a language feature per se. The example 
>> given is not exhaustive; there can be and are other uses of bridge methods.
>> 
>> Once the text is agreed to; I'll update the copyright year as well.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix typo noted by bpb.

OK just a couple markup comments, otherwise good.

src/java.base/share/classes/java/lang/reflect/Method.java line 597:

> 595:  * and {@code EnumSet} declares its language-level {@linkplain 
> java.util.EnumSet#clone() covariant override}
> 596:  * {@code public EnumSet clone() {...}}
> 597:  * If this technique was being used, the resulting class

Not sure what the preferred code block markup style is here; maybe `{@code 
...}` instead of ``.

-

Marked as reviewed by smarks (Reviewer).

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

Re: RFR: 8263102: Expand documention of Method.isBridge [v3]

[Stuart Marks]

On Tue, 9 Mar 2021 19:51:42 GMT, Stuart Marks  wrote:

>> Joe Darcy has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Respond to review feedback.
>
> src/java.base/share/classes/java/lang/reflect/Method.java line 579:
> 
>> 577:  * Bridge methods are used by Java compilers in various
>> 578:  * circumstances to span across differences in Java programming
>> 579:  * language semantics and JVM semantics.
> 
> Quibble: "span across" => "span"

Oh, also maybe need a  tag here.

-

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

Re: RFR: 8263102: Expand documention of Method.isBridge [v3]

[Stuart Marks]

On Tue, 9 Mar 2021 03:27:29 GMT, Joe Darcy  wrote:

>> The existing documentation of Method.isBridge isn't terribly helpful to the 
>> reader. This RFE proposes to given a common example of how bridge methods 
>> are used. The JLS does *not* have a section discussing bridge methods in 
>> detail; bridge methods are a compilation technique for lowering the Java 
>> language to the JVM, they are not a language feature per se. The example 
>> given is not exhaustive; there can be and are other uses of bridge methods.
>> 
>> Once the text is agreed to; I'll update the copyright year as well.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

Marked as reviewed by smarks (Reviewer).

-

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

Re: RFR: 8263102: Expand documention of Method.isBridge [v3]

[Stuart Marks]

On Tue, 9 Mar 2021 19:13:24 GMT, Joe Darcy  wrote:

>> src/java.base/share/classes/java/lang/reflect/Method.java line 589:
>> 
>>> 587:  * different return type, the virtual machine does not.
>>> 588:  * A
>>> 589:  * common case where covariant overrides are used is for a {@link
>> 
>> I think the example should be clearer because here, you don't show the code 
>> of EnumSet.clone().
>> I wonder if it's not easier if all the code is visible
>>   interface I {
>> Object m();
>>   }
>>   class A implements I {
>> String m() { return "hello"; }
>>   }
>> so you can explain that the VM do the dispatch on I::m()Object so the 
>> compiler has to insert a method A::m()Object,
>> the bridge method with this pseudo-code
>>   class A implements I {
>> /* bridge */ Object m() { return m(); } // calls m()String
>> String m()  { return "hello"; }
>>   }
>
> Hi Remi,
> 
> Thanks for the feedback. I did consider that kind of approach of an example 
> from scratch. I judged referencing instances of bridge methods in the base 
> module to be helpful to demonstrate it wasn't too esoteric of a feature in 
> terms of it is something you, as a developer, may already have seen. Also, 
> given the likely audience of the reading Class.isBridge, I didn't think a 
> stand-alone example was warranted.

The prose description here and is still rather clumsy, though. If you're going 
to use EnumSet.clone() as an example, maybe run with that and list the methods 
directly instead of saying "EnumSet.clone() returns EnumSet rather than 
Object", be more explicit. Maybe something like the following:

The Object class has the method:
 clone()```
whereas the EnumSet class has this method:
 clone()```
>From the JVM's perspective, the second method doesn't override the first, 
>because they have different return types. The Java compiler provides a proper 
>override by inserting the following bridge method into the EnumSet class:
 clone()```
that simply calls the second method and returns its result.

-

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

Re: RFR: 8263102: Expand documention of Method.isBridge [v3]

[Stuart Marks]

On Tue, 9 Mar 2021 03:27:29 GMT, Joe Darcy  wrote:

>> The existing documentation of Method.isBridge isn't terribly helpful to the 
>> reader. This RFE proposes to given a common example of how bridge methods 
>> are used. The JLS does *not* have a section discussing bridge methods in 
>> detail; bridge methods are a compilation technique for lowering the Java 
>> language to the JVM, they are not a language feature per se. The example 
>> given is not exhaustive; there can be and are other uses of bridge methods.
>> 
>> Once the text is agreed to; I'll update the copyright year as well.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

src/java.base/share/classes/java/lang/reflect/Method.java line 579:

> 577:  * Bridge methods are used by Java compilers in various
> 578:  * circumstances to span across differences in Java programming
> 579:  * language semantics and JVM semantics.

Quibble: "span across" => "span"

-

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

Re: RFR: 8263102: Expand documention of Method.isBridge [v2]

[Stuart Marks]

On Sat, 6 Mar 2021 17:44:18 GMT, Joe Darcy  wrote:

>> The existing documentation of Method.isBridge isn't terribly helpful to the 
>> reader. This RFE proposes to given a common example of how bridge methods 
>> are used. The JLS does *not* have a section discussing bridge methods in 
>> detail; bridge methods are a compilation technique for lowering the Java 
>> language to the JVM, they are not a language feature per se. The example 
>> given is not exhaustive; there can be and are other uses of bridge methods.
>> 
>> Once the text is agreed to; I'll update the copyright year as well.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Improve linkage for isSynethetic.

src/java.base/share/classes/java/lang/reflect/Method.java line 593:

> 591:  * result. (While the Java language specification forbids a class
> 592:  * declaring two methods with the same parameter types but a
> 593:  * different return type, the virtual machine does not.)

I'm of two minds about this. This is certainly a good example of a bridge 
method. It doesn't motivate _why_ a bridge method is necessary in this case. It 
think it's because the language says you should be able to write code like
EnumSet es2 = es.clone();
so there needs to be a method defined in the class file whose return type is 
assignment-compatible  with `EnumSet`. However, a `clone` method that returns 
`Object` is the only one that can override the `Object::clone` method. Thus, a 
something is necessary to span this gap -- a bridge method.

On the other hand, this might be too much detail for here. This is a really 
obscure location. It seems like somewhere else would be better, and where a 
full explanation with examples can be provided.

src/java.base/share/classes/java/lang/reflect/Method.java line 597:

> 595:  * Bridge methods may also be used by Java compiler in other
> 596:  * circumstances to span across difference in Java Language
> 597:  * semantics and JVM semantics.

If you decide to put less detail here, you could start with this statement. I 
think the main point is that there are some semantic gaps between methods in 
the Java language and in the JVM; bridge methods are necessary to "span" this 
gap. You might simply list some examples without explaining them fully.

Would this be "used by **a** Java compiler" or "used by **the** Java compiler?

-

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

Re: RFR: 8263190: Update java.io, java.math, and java.text to use instanceof pattern variable

[Stuart Marks]

On Mon, 8 Mar 2021 18:48:30 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.io`, 
> `java.math`, and `java.text` packages to make use of the `instanceof` pattern 
> variable?
> 
> Kind regards,
> Patrick

Marked as reviewed by smarks (Reviewer).

-

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

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v7]

[Stuart Marks]

On Thu, 4 Mar 2021 04:13:12 GMT, Ian Graves  wrote:

>> Modify the `unmodifiable*` methods in `java.util.Collections` to be 
>> idempotent. That is, when given an immutable collection from 
>> `java.util.ImmutableCollections` or `java.util.Collections`, these methods 
>> will return the reference instead of creating a new immutable collection 
>> that wraps the existing one.
>
> Ian Graves has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Revert "Avoid wrapping subclasses where relevant. Updated tests."
>   
>   This reverts commit dda174c582590827cad3f4006a8ff9e7dd8a51ab.

Marked as reviewed by smarks (Reviewer).

-

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

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v5]

[Stuart Marks]

On Fri, 26 Feb 2021 21:37:14 GMT, Stuart Marks  wrote:

>> Ian Graves has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Test refactoring. Adding implNote to modified methods
>
> The `@implNote` additions are good, and the test rewrite looks good too.

Hm. I had thought of this previously but I was a bit suspicious, and it didn't 
seem like it would make much difference, so I didn't say anything. But thinking 
about this further, the following issues arose:

1. Suppose you have an UnmodifiableSortedSet and call unmodifiableSet() on it, 
it returns its argument, and then you hand it out. Its static type is Set but 
it's really a SortedSet, which means somebody can downcast it and get its 
comparator. This leaks some information. Offhand this doesn't look dangerous, 
but it's a bit of a surprise.

2. Thinking about this further, this allows heap pollution. (Ian, turns out our 
conversation from the other day wasn't just an idle digression.) If we have 
class X and class Y extends X, then `SortedSet` cannot safely be cast to an 
unmodifiable `SortedSet`. That's because comparator() will return 
`Comparator` which is incorrect, since the actual comparator might 
be of type `Comparator`. Actually the headSet(), subSet(), and tailSet() 
methods also cause problems, because they both consume and produce the 
collection element type E.

3. This can actually happen in practice with code in the JDK! 
PriorityBlockingQueue's copy constructor does exactly the above. It takes a 
Collection and does an instanceof check to see if it's a SortedSet; if it is, 
it does a downcast and uses its comparator. Thus if we do the following:

SortedSet set1 = new TreeSet<>(Integer::compare);
Set set2 = Collections.unmodifiableSet(set1); // hypothetical 
version that returns its argument
PriorityBlockingQueue pbq = new PriorityBlockingQueue<>(set2);
pbq.addAll(Arrays.asList(1.0, 2.0));

this compiles without warnings, but it results in ClassCastException. The 
culprit is the new upcast that potentially allows `SortedSet` to 
be cast to `Set`, which is slipped in under the already-existing warnings 
suppression.

In any case, the extra checking in the unmodifiableSortedSet and -Map methods 
needs to be taken out. Offhand I don't know if there's a similar issue between 
unmodifiableSortedSet and a NavigableSet (resp., Map), but on general principle 
I'd say to take it out too. It's likely not buying much anyway.

The UnmodifiableList and UnmodifiableRandomAccessList stuff should stay, since 
that's how the RandomAccess marker interface is preserved.

-

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

Re: RFR: 8261862: Expand discussion of rationale for BigDecimal equals/compareTo semantics

[Stuart Marks]

On Wed, 3 Mar 2021 07:20:03 GMT, Joe Darcy  wrote:

> I considered @stuart-marks previous suggestion during the code review of 
> JDK-8261123 to include a more explicit discussion of why, say, different 
> representations of 2 should not be regarded as equivalent. After 
> contemplating several alternatives, I didn't find anything simpler than 
> Stuart's 2/3 example so I used that as seen in the diff.
> 
> A short digression, BigDecimal supports both fixed-point style and 
> floating-point style rounding. Floating-point rounding primarily replies on 
> the number of precision digits, regards of their scale, while fixed-point 
> style rounding prioritizes the scale. The number of digits of eventual output 
> is a function of number number of digits in the inputs and the number of 
> precision digits in a floating-point style rounding. A floating-point style 
> rounding has a preferred scale, rather than a fixed scale based on the 
> inputs. The fixed-point style divide method used in the example has a scale 
> based on the dividend, allowing a relatively simple expression to show a 
> distinction between 2.0 and 2.00.

Marked as reviewed by smarks (Reviewer).

src/java.base/share/classes/java/math/BigDecimal.java line 3155:

> 3153:  * {@code new BigDecimal("2.00").divide(BigDecimal.valueOf(3),
> 3154:  * HALF_UP)} which evaluates to 0.67.
> 3155:  *

Should this be in an `@apiNote`?

I'm not sure adding the boldface 0.**6**7 is helpful; 0.7 is self-evidently 
unequal to 0.67.

-

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

Integrated: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Fri, 4 Dec 2020 06:50:14 GMT, Stuart Marks  wrote:

> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
> exception message, and adds a test. In addition to some renaming and a bit of 
> refactoring of the actual code, I also made two changes of substance to the 
> code:
> 
> 1. I fixed a problem with overflow checking. In the original code, if 
> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
> method could return a negative value. It turns out that writing tests helps 
> find bugs!
> 
> 2. Under the old policy, if oldLength and minGrowth required a length above 
> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
> allocate an array of that length will almost certainly cause the Hotspot to 
> throw OOME because its implementation limit was exceeded. Instead, if the 
> required length is in this range, this method returns that required length.
> 
> Separately, I'll work on retrofitting various call sites around the JDK to 
> use this method.

This pull request has now been integrated.

Changeset: f18c0192
Author:Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/f18c0192
Stats: 171 lines in 4 files changed: 135 ins; 2 del; 34 mod

8247373: ArraysSupport.newLength doc, test, and exception message

Reviewed-by: rriggs, psandoz, martin, prappo

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v5]

[Stuart Marks]

On Tue, 2 Mar 2021 15:05:51 GMT, Pavel Rappo  wrote:

>> Stuart Marks has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Update copyright year.
>
> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 579:
> 
>> 577: /**
>> 578:  * A soft maximum array length imposed by array growth computations.
>> 579:  * Some JVMs (such as Hotspot) have an implementation limit that 
>> will cause
> 
> The numbers of spellings "HotSpot" to that of "Hotspot" in the JDK codebase 
> is 10 to 1 respectively. Also, on my machine I can see this:
> 
> $ java --version
> java 15 2020-09-15
> Java(TM) SE Runtime Environment (build 15+36-1562)
> Java HotSpot(TM) 64-Bit Server VM (build 15+36-1562, mixed mode, sharing)

Fixed.

> test/jdk/jdk/internal/util/ArraysSupport/NewLength.java line 100:
> 
>> 98: int r = ArraysSupport.newLength(old, min, pref);
>> 99: fail("expected OutOfMemoryError, got normal return value of 
>> " + r);
>> 100: } catch (OutOfMemoryError success) { }
> 
> Consider using `expectThrows` or `assertThrows` from `org.testng.Assert`.

Good point. However, I actually tried to use assertThrows, but there doesn't 
seem to be a way to get the unexpected return value into the error message. I 
think having this value in the test output is important for diagnosing failures.

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v6]

[Stuart Marks]

> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
> exception message, and adds a test. In addition to some renaming and a bit of 
> refactoring of the actual code, I also made two changes of substance to the 
> code:
> 
> 1. I fixed a problem with overflow checking. In the original code, if 
> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
> method could return a negative value. It turns out that writing tests helps 
> find bugs!
> 
> 2. Under the old policy, if oldLength and minGrowth required a length above 
> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
> allocate an array of that length will almost certainly cause the Hotspot to 
> throw OOME because its implementation limit was exceeded. Instead, if the 
> required length is in this range, this method returns that required length.
> 
> Separately, I'll work on retrofitting various call sites around the JDK to 
> use this method.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix spelling of HotSpot

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1617/files
  - new: https://git.openjdk.java.net/jdk/pull/1617/files/8892eb47..68318d46

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1617=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1617=04-05

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

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v5]

[Stuart Marks]

> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
> exception message, and adds a test. In addition to some renaming and a bit of 
> refactoring of the actual code, I also made two changes of substance to the 
> code:
> 
> 1. I fixed a problem with overflow checking. In the original code, if 
> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
> method could return a negative value. It turns out that writing tests helps 
> find bugs!
> 
> 2. Under the old policy, if oldLength and minGrowth required a length above 
> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
> allocate an array of that length will almost certainly cause the Hotspot to 
> throw OOME because its implementation limit was exceeded. Instead, if the 
> required length is in this range, this method returns that required length.
> 
> Separately, I'll work on retrofitting various call sites around the JDK to 
> use this method.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  Update copyright year.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1617/files
  - new: https://git.openjdk.java.net/jdk/pull/1617/files/c520e8e8..8892eb47

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1617=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1617=03-04

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

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v4]

[Stuart Marks]

> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
> exception message, and adds a test. In addition to some renaming and a bit of 
> refactoring of the actual code, I also made two changes of substance to the 
> code:
> 
> 1. I fixed a problem with overflow checking. In the original code, if 
> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
> method could return a negative value. It turns out that writing tests helps 
> find bugs!
> 
> 2. Under the old policy, if oldLength and minGrowth required a length above 
> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
> allocate an array of that length will almost certainly cause the Hotspot to 
> throw OOME because its implementation limit was exceeded. Instead, if the 
> required length is in this range, this method returns that required length.
> 
> Separately, I'll work on retrofitting various call sites around the JDK to 
> use this method.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  Add newline at end of file.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1617/files
  - new: https://git.openjdk.java.net/jdk/pull/1617/files/6710004c..c520e8e8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1617=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1617=02-03

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

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v3]

[Stuart Marks]

> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
> exception message, and adds a test. In addition to some renaming and a bit of 
> refactoring of the actual code, I also made two changes of substance to the 
> code:
> 
> 1. I fixed a problem with overflow checking. In the original code, if 
> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
> method could return a negative value. It turns out that writing tests helps 
> find bugs!
> 
> 2. Under the old policy, if oldLength and minGrowth required a length above 
> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
> allocate an array of that length will almost certainly cause the Hotspot to 
> throw OOME because its implementation limit was exceeded. Instead, if the 
> required length is in this range, this method returns that required length.
> 
> Separately, I'll work on retrofitting various call sites around the JDK to 
> use this method.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  Update with recommendations from Martin Buchholz.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1617/files
  - new: https://git.openjdk.java.net/jdk/pull/1617/files/bacb5f91..6710004c

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

  Stats: 12 lines in 2 files changed: 1 ins; 4 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1617.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1617/head:pull/1617

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

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v5]

[Stuart Marks]

On Fri, 26 Feb 2021 20:15:19 GMT, Ian Graves  wrote:

>> Modify the `unmodifiable*` methods in `java.util.Collections` to be 
>> idempotent. That is, when given an immutable collection from 
>> `java.util.ImmutableCollections` or `java.util.Collections`, these methods 
>> will return the reference instead of creating a new immutable collection 
>> that wraps the existing one.
>
> Ian Graves has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Test refactoring. Adding implNote to modified methods

The `@implNote` additions are good, and the test rewrite looks good too.

-

Marked as reviewed by smarks (Reviewer).

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

Re: RFR: 8252399: Update mapMulti documentation to use type test pattern instead of instanceof once JEP 375 exits preview [v7]

[Stuart Marks]

On Fri, 12 Feb 2021 20:27:00 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my changeset for JDK-8252399: 'Update mapMulti 
>> documentation to use type test pattern instead of instanceof once JEP 375 
>> exits preview' ?
>> 
>> This change updates the example code displayed in the API documentation for 
>> mapMulti to use the type test pattern introduced in 
>> [JEP375](https://openjdk.java.net/jeps/375).
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8252399: Converted JavadocExamples to test

Marked as reviewed by smarks (Reviewer).

src/java.base/share/classes/java/util/stream/Stream.java line 410:

> 408:  *
> 409:  * public static void main(String[] args) {
> 410:  * var nestedList = ...;

I think it would be good to expand the RHS to use what you use in the test, 
namely

var nestedList = List.of(1, List.of(2, List.of(3, 4)), 5);

That would clarify the example quite a bit. Similar for the numbers example 
above.

src/java.base/share/classes/java/util/stream/Stream.java line 414:

> 412:  * }
> 413:  * }
> 414:  * }

I'd suggest putting a comment somewhere outside the javadoc that points to the 
test and has a statement similar to the one in the test, about keeping the 
example here in synch with the test. I'm not sure what the best way is to do 
this though. It probably cannot be after the javadoc comment, because the 
javadoc comment needs to be immediately prior to the method declaration. Maybe 
a // comment above the doc comment?

-

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

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]

[Stuart Marks]

On Tue, 23 Feb 2021 23:32:01 GMT, Stuart Marks  wrote:

>>> > Is there any behavior change here that merits a CSR review?
>>> 
>>> Maybe. The one observable change is that calling `Collections.bar(foo)` 
>>> with a `foo` that is already a `bar` will return the instance rather than 
>>> unnecessarily wrap it. This could change semantics in applications 
>>> inadvertently or deliberately relying on identity.
>> 
>> Yes. The CSR was to consider primarily this case. Probably out of an 
>> abundance of caution here. @stuart-marks may have another case to consider.
>
>> Is there any behavior change here that merits a CSR review?
> 
> Yes. See my comments in the bug report:
> 
> https://bugs.openjdk.java.net/browse/JDK-6323374?focusedCommentId=14296330=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14296330
> 
> There is not only the issue of the identity of the object returned, but the 
> change is also observable in the serialized form. Most people would consider 
> the change (less nesting) to be an improvement, but the change is observable, 
> and as we know any observable behavior can become depended upon by 
> applications.

Code changes all look good. I'm thinking that we should add `@implNote` clauses 
to all the docs of the affected methods, saying something like "This method may 
return its argument if it is already unmodifiable." Usually it's reasonable to 
leave these kinds of behaviors unspecified (and we do so elsewhere) but since 
this is a change in long-standing behavior, it seems reasonable to highlight it 
explicitly. I don't think we want to specify it though, because of the issue 
with ImmutableCollections (as discussed previously) and possible future tuning 
of behavior regarding the various Set and Map subinterfaces. (For example, 
C.unmodifiableSet(arg) could return arg if it's an UnmodifiableNavigableSet.)

The test seems to have a lot of uncomfortable dependencies, both explicit and 
implicit, on the various ImmutableCollection and UnmodifiableX implementation 
classes. Would it be sufficient to test various instances for reference 
equality and inequality instead? For example, something like

var list0 = List.of();
var list1 = Collections.unmodifiableList(list0);
var list2 = Collections.unmodifiableList(list1);
assertNotSame(list0, list1);
assertSame(list1, list2);

This would avoid having to write test cases that cover various internal 
classes. The ImmutableCollections classes have been reorganized in the past, 
and while we don't have any plans to do so again at the moment, there is always 
the possibility of it happening again.

One could write out all the different cases "by hand" but there are rather a 
lot of them. It might be fruitful to extract the "wrap once, wrap again, 
assertNotSame, assertSame" logic into a generic test and drive it somehow with 
a data provider that provides the base instance and a wrapper function.

-

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

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]

[Stuart Marks]

On Tue, 23 Feb 2021 16:27:06 GMT, Ian Graves  wrote:

> Is there any behavior change here that merits a CSR review?

Yes. See my comments in the bug report:

https://bugs.openjdk.java.net/browse/JDK-6323374?focusedCommentId=14296330=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14296330

There is not only the issue of the identity of the object returned, but the 
change is also observable in the serialized form. Most people would consider 
the change (less nesting) to be an improvement, but the change is observable, 
and as we know any observable behavior can become depended upon by applications.

-

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

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]

[Stuart Marks]

On Fri, 19 Feb 2021 01:52:51 GMT, liach  
wrote:

>> Maybe it is not correct for UnmodifiableEntrySet::contains to short circuit? 
>>  What if the implementation was changed to:
>> 
>> `public boolean contains(Object o) {
>> if (!(o instanceof Map.Entry))
>> return c.contains(o); //false, NPE, or CCE
>> return c.contains(
>> new UnmodifiableEntry<>((Map.Entry) o));
>> }`
>
> This however changes the behavior of unmodifiable maps compared to before; 
> i.e. before for other entry sets, they could not throw exception if the 
> object passed was not map entry; now they can.

Yes, it seems reasonable to decouple the `ImmutableCollections` issues from 
this change.

-

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

Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v6]

[Stuart Marks]

On Fri, 12 Feb 2021 22:52:06 GMT, Joe Darcy  wrote:

>> A follow-up of sorts to JDK-8257086, this change aims to improve the 
>> discussion of the relationship between Object.equals and compareTo and 
>> compare methods. The not-consistent-with-equals natural ordering of 
>> BigDecimal get more explication too. While updating Object, I added some 
>> uses of apiNote and implSpec, as appropriate. While a signum method did not 
>> exist when Comparable was added, it has existed for many years so I removed 
>> obsolete text that defined a "sgn" function to compute signum.
>> 
>> Once the changes are agreed to, I'll reflow paragraphs and update the 
>> copyright years.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix typo.

Marked as reviewed by smarks (Reviewer).

src/java.base/share/classes/java/math/BigDecimal.java line 99:

> 97:  * hold. The results of methods like {@link scale} and {@link
> 98:  * unscaledValue} will differ for numerically equal values with
> 99:  * different representations.

While this text is correct and reasonable, it doesn't really explain _why_ 
equals() considers the representation. One might assume incorrectly that the 
representation is internal only and doesn't affect computations. Of course it 
does affect computations, which is why I suggested the example. Maybe the 
example doesn't belong right here, in which case it's reasonable for this 
change to proceed. I think it would be good to put an example _somewhere_ of 
non-substitutability of numerical values with different representations. Maybe 
we could handle that separately.

-

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

Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]

[Stuart Marks]

On Fri, 12 Feb 2021 22:12:30 GMT, Joe Darcy  wrote:

>> src/java.base/share/classes/java/lang/Comparable.java line 90:
>> 
>>> 88:  * of the {@code equals} method and the equivalence classes defined by
>>> 89:  * the quotient of the {@code compareTo} method are the same.
>>> 90:  *
>> 
>> I think in both cases it should be "the equivalence class defined by" 
>> That is, "equivalence class" should be singular. But there are two of them, 
>> so the sentence still properly concludes "... are the same."
>
> An equivalence relation defines a *set* of equivalence classes which 
> partition the objects the relation operates on. For example, the "same signum 
> value" equivalence relation on integers has three equivalence classes :
> 1) negative nonzero numbers (corresponding to signum == -1)
> 2) zero (corresponding to signum = 0)
> 3) positive nonzero numbers (corresponding to signum ==1)

OK, got it.

-

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

Re: RFR: 8260221: java.util.Formatter throws wrong exception for mismatched flags in %% conversion

[Stuart Marks]

On Wed, 3 Feb 2021 22:42:00 GMT, Ian Graves  wrote:

> Updating the specification to reflect well-established behavior in Formatter 
> when incorrect flags used for `%`.

Marked as reviewed by smarks (Reviewer).

-

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

Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]

[Stuart Marks]

On Thu, 11 Feb 2021 04:24:40 GMT, Stuart Marks  wrote:

>> Joe Darcy has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix typos in javadoc tags found during review.
>
> src/java.base/share/classes/java/math/BigDecimal.java line 97:
> 
>> 95:  * contrast, the {@link equals equals} method requires both the
>> 96:  * numerical value and representation to be the same for equality to
>> 97:  * hold.
> 
> Note, discussing "representation" is ok here since the context is discussing 
> the representation of BigDecimal (in contrast to the text in Comparable).

It might be reasonable to add a bit of rationale here and give an example. I 
think the reason that members of the same cohort might not be considered 
`equals()` to one another is that they are not substitutable. For example, 
consider 2.0 and 2.00. They are members of the same cohort, because

new BigDecimal("2.0").compareTo(new BigDecimal("2.00")) == 0

is true. However,

new BigDecimal("2.0").equals(new BigDecimal("2.00"))

is false. They aren't considered `equals()` because they aren't substitutable, 
since using them in an arithmetic expression can give different results. For 
example:

new BigDecimal("2.0").divide(new BigDecimal(3), RoundingMode.HALF_UP)
==> 0.7

new BigDecimal("2.00").divide(new BigDecimal(3), RoundingMode.HALF_UP)
==> 0.67

I think that's the right rationale, and a reasonable example to illustrate it. 
Edit to taste. I think it would be good to have material like this, though, 
because people's immediate reaction to BD being inconsistent with equals is 
"well that's wrong." Well, no, it's right, and I think this is the reason.

-

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

Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]

[Stuart Marks]

On Wed, 10 Feb 2021 01:49:55 GMT, Joe Darcy  wrote:

>> A follow-up of sorts to JDK-8257086, this change aims to improve the 
>> discussion of the relationship between Object.equals and compareTo and 
>> compare methods. The not-consistent-with-equals natural ordering of 
>> BigDecimal get more explication too. While updating Object, I added some 
>> uses of apiNote and implSpec, as appropriate. While a signum method did not 
>> exist when Comparable was added, it has existed for many years so I removed 
>> obsolete text that defined a "sgn" function to compute signum.
>> 
>> Once the changes are agreed to, I'll reflow paragraphs and update the 
>> copyright years.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix typos in javadoc tags found during review.

Overall very nice. Mostly my comments are wordsmithing. Two issues are worth 
considering; see detailed comments inline.

1) Do we want a general statement on the stability of the value returned by 
toString()?

2) Add rationale and example for why BigDecimal's natural ordering is 
inconsistent with equals.

src/java.base/share/classes/java/lang/Comparable.java line 68:

> 66:  * orderings that are consistent with equals.  One exception is
> 67:  * {@link java.math.BigDecimal}, whose {@linkplain 
> java.math.BigDecimal#compareTo natural ordering} equates
> 68:  * {@code BigDecimal} objects with equal numerical values and different 
> representations

Nothing here talks about the representation of BigDecimal. I think it would be 
preferable to say that in BigDecimal, the `equals()` method considers 4.0 and 
4.00 unequal because they have different precisions; however, `compareTo()` 
considers them equal because it considers only their numerical values.

src/java.base/share/classes/java/lang/Comparable.java line 90:

> 88:  * of the {@code equals} method and the equivalence classes defined by
> 89:  * the quotient of the {@code compareTo} method are the same.
> 90:  *

I think in both cases it should be "the equivalence class defined by" That 
is, "equivalence class" should be singular. But there are two of them, so the 
sentence still properly concludes "... are the same."

src/java.base/share/classes/java/lang/Comparable.java line 110:

> 108:  * {@link Integer#signum signum}{@code (x.compareTo(y)) == 
> -signum(y.compareTo(x))}
> 109:  * for all {@code x} and {@code y}.  (This
> 110:  * implies that {@code x.compareTo(y)} must throw an exception iff

I'd suggest replacing "iff" with "if and only if" because it looks like a typo, 
and writing out the long form emphasizes the bidirectional nature of the 
implication.

src/java.base/share/classes/java/lang/Object.java line 135:

> 133:  * into equivalence classes; all the members of an
> 134:  * equivalence class are equal to each other. The equal objects
> 135:  * are substitutable for each other, at least for some purposes.

Since the preceding sentence mentions the members of an equivalence class, it 
might read better to say "Members are substitutable..." or possibly "Members of 
an equivalence class are substitutable"

src/java.base/share/classes/java/lang/Object.java line 149:

> 147:  *
> 148:  * @apiNote
> 149:  * Note that it is generally necessary to override the {@link 
> hashCode hashCode}

The `@apiNote` introduces the paragraph with a subhead "API Note:" so it's a 
bit redundant to say "Note that" Maybe just start with "It is generally 
necessary"

src/java.base/share/classes/java/lang/Object.java line 236:

> 234:  * be a concise but informative representation that is easy for a
> 235:  * person to read.
> 236:  * It is recommended that all subclasses override this method.

Do we want to have a general note here or somewhere that the exact format of 
the result of `toString()` is generally not stable, and that it is subject to 
change without notice?

src/java.base/share/classes/java/math/BigDecimal.java line 97:

> 95:  * contrast, the {@link equals equals} method requires both the
> 96:  * numerical value and representation to be the same for equality to
> 97:  * hold.

Note, discussing "representation" is ok here since the context is discussing 
the representation of BigDecimal (in contrast to the text in Comparable).

src/java.base/share/classes/java/util/Comparator.java line 95:

> 93:  * equals, the equivalence classes defined by the equivalence relation
> 94:  * of the {@code equals} method and the equivalence classes defined by
> 95:  * the quotient of the {@code compare} method are the same.

As before, suggest "equivalence class" be singular in both cases.

src/java.base/share/classes/java/util/Comparator.java line 159:

> 157:  * and it imposes the same ordering as this comparator.  Thus,
> 158:  * {@code comp1.equals(comp2)} implies that {@code 
> signum(comp1.compare(o1,
> 159:  * o2))==signum(comp2.compare(o1, o2))} for every 

Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]

[Stuart Marks]

On Wed, 10 Feb 2021 02:55:14 GMT, Brian Burkhalter  wrote:

>> This is the exact text recommended in java.lang.Comparable when a type's 
>> natural ordering is inconsistent with equals. The statement to that effect 
>> at the top of BigDecimal didn't use that exact wording
>
> Okay, I see.

The note itself should be here, but it's demarcated with an `@apiNote` tag. 
This introduces a subhead "API Note:" in the rendered javadoc. Thus, it's not 
necessary to start the text with "Note:".

-

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

Re: RFR: 8261096: Convert jlink tool to use Stream.toList()

[Stuart Marks]

On Thu, 4 Feb 2021 22:45:10 GMT, Ian Graves  wrote:

> A subtask [JDK-8260559](https://bugs.openjdk.java.net/browse/JDK-8260559). 
> This is an enhancement to update jlink's usage of `Collections.toList()` to 
> `Stream.toList()`.

Aside from the simplification issues, the Collectors.toList() => 
Stream.toList() conversions look fine.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java line 569:

> 567: ? 
> Stream.of(Arrays.copyOfRange(args, i, args.length))
> 568: .toList()
> 569: : Collections.emptyList();

This is probably out of scope for a simple toList() conversion, but there are 
some additional ways this code could be simplified. It's possible to stream a 
subrange of an array directly:

`Arrays.stream(args, i, args.length).toList()`

Since a zero-length array subrange results in a valid zero-length list, the 
enclosing ternary is unnecessary:

`return Arrays.stream(args, ++i, args.length).toList();`

It's also possible to do this without using streams at all,

`return List.copyOf(Arrays.asList(args).subList(++i, args.length));`

but this actually seems more cumbersome than streaming the array subrange. In 
any case, perhaps this sort of cleanup could be reserved for a future changeset 
from the jlink maintainer.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java line 579:

> 577: return Stream.of(Arrays.copyOfRange(args, i, 
> args.length))
> 578:  .toList();
> 579: }

As above, stream an array subrange.

-

Marked as reviewed by smarks (Reviewer).

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

Re: RFR: JDK-8257086: Clarify differences between {Float, Double}.equals and == [v5]

[Stuart Marks]

On Sat, 30 Jan 2021 01:58:02 GMT, Joe Darcy  wrote:

>> Updates to the specifications of Double.{equals, compareTo} to explain more 
>> explicitly why the obvious wrappers around "==" and "<"/"==" are not 
>> sufficient for floating-point values.
>> 
>> Once the wording is worked out, I'll replicate it for the analogous methods 
>> on Float.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix typos noticed by @bplb.

I did another full reread. Everything looks good!

-

Marked as reviewed by smarks (Reviewer).

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

Re: Why does Set.of disallow duplicate elements?

[Stuart Marks]

Indeed it's the case that a varargs method can't determine whether it was called 
with several explicit arguments or whether it was called with an array. However, 
that doesn't have any bearing on whether or not Set.of rejects duplicates.


The model for Set.of is to support a collection-literal-like syntax where the 
programmer can write an arbitrary number of elements in the source code for 
inclusion in the set. Here's an example (though it uses Map.ofEntries instead of 
Set.of, the same rationale applies):



Map tokens = Map.ofEntries(
entry("@", AT),
entry("|", VERTICAL_BAR),
entry("#", HASH),
entry("%", PERCENT),
entry(":", COLON),
entry("^", CARET),
entry("&", AMPERSAND),
entry("|", EXCLAM),
entry("?", QUESTION),
entry("$", DOLLAR),
entry("::",PAAMAYIM_NEKUDOTAYIM),
entry("=", EQUALS),
entry(";", SEMICOLON)
);


This errors out instead of silently dropping one of the entries.

As an optimization, the API provides several fixed-arg overloads of Set.of. With few 
arguments, the fixed-arg methods are called. If more arguments are added, at a 
certain point it transparently switches to the varargs form. "Transparently" means 
that you can't tell (without counting the arguments) whether a fixed-arg or varargs 
form of Set.of will be called. You don't want the duplicate rejection semantics to 
change if you add or remove an argument that happens to cross the fixed/varargs 
threshold. Thus, Set.of rejects duplicates, whether in fixed or varargs form.


Set.copyOf(Arrays.asList(...)) is the best way to deduplicate an explicit list of 
elements into a set.


s'marks




On 2/1/21 3:01 PM, Aaron Scott-Boddendijk wrote:

  Dave,

|| Dave said...
|| Okay, I understand this reasoning, but when you want to construct a Set
from an array, you might be tempted to use Set.of(...) because it looks
like it supports an
|| array and indeed, you can do Set.of(new int[] {1, 2 }) I believe?
||
|| Maybe this is just a quirk because of how varargs work.

| Rémi said...
| Set.of(int[]) will call Set.of(E) with E being an int[].
| but
| Set.of(new Integer[] { ... }) calls Set.of(...).
|
| Yes, exactly, it's a known issue with varargs, you have no way to say, i
don't want this varargs to be called with an array.

I think the confusion is the interaction of boxing and varargs.


List list = List.of(1, 2);


is actually, once auto-boxing is applied by the compiler, executed as...


List list = List.of(Integer.valueOf(1), Integer.valueOf(2));


So the equivalent explicit array form should use `Integer[]` not `int[]`...


Integer[] numbers = new Integer[] {1, 2};
List list = List.of(numbers);


Interestingly, if you actually wanted a `List` you would then
need to say


Integer[] numbers = new Integer[] {1, 2};
List list = List.of(numbers);


Which is explicitly telling the compiler what the type arguments are for
this invocation of the generic method 'of'' (rather than allowing it to use
type-inference)

Regarding the use of `Set.copyOf(Arrays.asList(...))`. I do wonder about
improving the ceremony (because I agree that we want an obvious way of
getting immutable Sets from non-unique inputs) by following the pattern
presented in Optional (`Optional.of` and `Optional.ofNullable`) and
providing `Set.of` and `Set.ofMaybeUnique` (better name needed -
'ofOptionallyUnique'?) - to which the implementation could just be
`Set.copyOf(Arrays.asList(args))` (unless a more efficient path proves
valuable).

`Arrays.asList(...array...)` is not all that expensive. It is _not_ an
ArrayList but a much simpler type with rather trivial implementations for
most methods (and 'always throws' implementations for methods that are
unsupported). So not only does it mean that there's no copying occuring to
make the list but it's even possible that JIT manages enough specialisation
and inlining to elide the allocation entirely (though in practice this
doesn't occur as often as we might like).

--
Aaron Scott-Boddendijk

On Mon, Feb 1, 2021 at 10:35 AM  wrote:







De: "dfranken jdk" 
À: "Remi Forax" 
Cc: "core-libs-dev" 
Envoyé: Dimanche 31 Janvier 2021 13:54:44
Objet: Re: Why does Set.of disallow duplicate elements?




BQ_BEGIN

Okay, I understand this reasoning, but when you want to construct a Set
from an array, you might be tempted to use Set.of(...) because it looks
like it supports an array and indeed, you can do Set.of(new int[] {1, 2 })
I believe?

BQ_END

Set.of(int[]) will call Set.of(E) with E being an int[].
but
Set.of(new Integer[] { ... }) calls Set.of(...).


BQ_BEGIN


Maybe this is just a quirk because of how varargs work.

BQ_END

Yes, exactly, it's a known issue with varargs, you have no way to say, i
don't want this varargs to be called with an array.


BQ_BEGIN


I wondered if there was a canonical way to create a Set from an array, but
couldn't find it, maybe I am missing something?
I did notice Arrays.asList exists (which makes 

Re: RFR: JDK-8257086: Clarify differences between {Float, Double}.equals and == [v3]

[Stuart Marks]

On Fri, 29 Jan 2021 17:31:09 GMT, Joe Darcy  wrote:

>> Updates to the specifications of Double.{equals, compareTo} to explain more 
>> explicitly why the obvious wrappers around "==" and "<"/"==" are not 
>> sufficient for floating-point values.
>> 
>> Once the wording is worked out, I'll replicate it for the analogous methods 
>> on Float.
>
> Joe Darcy has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains six additional commits since 
> the last revision:
> 
>  - Merge branch 'master' into JDK-8257086
>  - Update reworked wording from @smarks.
>  - Merge branch 'master' into JDK-8257086
>  - Merge branch 'master' into JDK-8257086
>  - Fix whitespace
>  - Initial work for JDK-8257086.

Overall good! Just some whitespace errors and a couple typos as noted.

src/java.base/share/classes/java/lang/Double.java line 117:

> 115:  * 
> 116:  * To provide the appropriate semantics for {@code equals} and {@code
> 117:  * compareTo} methods, those methods cannot simply to wrappers around

I think this should be "cannot simply be wrappers".

-

Marked as reviewed by smarks (Reviewer).

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

Re: RFR: JDK-8257086: Clarify differences between {Float, Double}.equals and ==

[Stuart Marks]

On Thu, 28 Jan 2021 07:15:05 GMT, Joe Darcy  wrote:

>> Great, let me know if you'd like me to provide some text for any particular 
>> topics in this area.
>
>> 
>> 
>> Great, let me know if you'd like me to provide some text for any particular 
>> topics in this area.
> 
> Before sending out another iteration in code, there the draft javadoc text of 
> a discussion at the end of the class-level discussion of java.lang.Double:
> 
>  * Floating-point Equality, Equivalence,
>  * and Comparison
>  *
>  * IEEE 754 floating-point values include finite nonzero values,
>  * signed zeros ({@code +0.0} and {@code -0.0}), signed infinities
>  * {@linkplain Double#POSITIVE_INFINITY positive infinity} and
>  * {@linkplain Double#NEGATIVE_INFINITY negative infinity}), and
>  * {@linkplain Double#NaN NaN} (not-a-number).
>  *
>  * An equivalence relation on a set of values is a boolean
>  * relation on pairs of values that is reflexive, symmetric, and
>  * transitive. A set can have multiple equivalence relations defined
>  * for it. For example, {@link java.util.HashMap HashMap} compares
>  * keys in the set of objects using the equivalence relation of {@link
>  * Object#equals Object.equals} while {@link java.util.IdentityHashMap
>  * IdentityHashMap} compares keys in the set of objects using the
>  * equivalence relation of reference equality.
>  * 
>  * For floating-point values to be used in data structures like
>  * sets and maps, the {@linkplain Double#equals equals} or {@linkplain
>  * Double#compareTo comparison} method must satisfy the usual
>  * requirements of being an equivalence relation or analogous
>  * requirement for comparison methods. However, surprisingly, the
>  * built-in {@code ==} operation on floating-point values does
>  * not implement an equivalence relation. Despite not
>  * defining an equivalence relation, the semantics of the IEEE 754
>  * {@code ==} operator were deliberately designed to meet other needs
>  * of numerical computation. There are two exceptions where the
>  * properties of an equivalence relations are not satisfied by {@code
>  * ==} on floating-point values:
>  *
>  * 
>  *
>  * If {@code v1} and {@code v2} are both NaN, then {@code v1
>  * == v2} has the value {@code false}. Therefore, for two NaN
>  * arguments the reflexive property of an equivalence
>  * relation is not satisfied by the {@code ==} operator.
>  *
>  * If {@code v1} represents {@code +0.0} while {@code v2}
>  * represents {@code -0.0}, or vice versa, then {@code v1 == v2} has
>  * the value {@code true} even though {@code +0.0} and {@code -0.0}
>  * are distinguishable under various floating-point operations. For
>  * example, {@code 1.0/+0.0} evaluates to positive infinity while
>  * {@code 1.0/-0.0} evaluates to negative infinity and
>  * positive infinity and negative infinity are neither equal to each
>  * other nor equivalent to each other.
>  *
>  * 
>  *
>  * For ordered comparisons using the built-in comparison operator
>  * ({@code <}, {@code <=}, etc.), NaN values have another anomalous
>  * situation: a NaN is neither less than, greater than, nor equal to
>  * any value, including itself. This means the trichotomy of
>  * comparison does not hold.
>  * 
>  * To provide the appropriate semantics for {@code equals} and {@code
>  * compareTo} methods, those methods cannot simply to wrappers around
>  * {@code ==} or ordered comparison operations. Instead, {@link
>  * Double#equals equals} defines NaN arguments to be equal to each
>  * other and defines {@code +0.0} to not be equal to {@code
>  * -0.0}, restoring reflexivity. For comparisons, {@link
>  * Double#compareTo compareTo} defines a total order where {@code
>  * -0.0} is less than {@code +0.0} and where a NaN is equal to itself
>  * and considered greater than positive infinity.
>  *
>  * The operational semantics of {@code equals} and {@code
>  * compareTo} are expressed in terms of {@linkplain doubleToLongBigs
>  * bit-wise converting} the floating-point values to integral values 
>  *
>  * The natural ordering implemented by {@link compareTo
>  * compareTo} is {@linkplain Comparable consistent with equals}; that
>  * is values are only reported as equal by {@code equals} if {@code
>  * compareTo} on those objects returns zero.
>  *
>  * @jls 4.2.3 Floating-Point Types, Formats, and Values
>  * @jls 4.2.4. Floating-Point Operations
>  * @jls 15.21.1 Numerical Equality Operators == and !=
> 
> Comments?
> 
> Thanks,

I took the liberty of making an editing pass over the proposed text. Along with 
a few editorial and markup adjustments, the key changes are as follows:

1) I moved discussion of interaction with Set and Map to the end, after all the 
issues have been set up.

2) I added the concept of substitution alongside the equivalence relation. 
AFAIK substitution (or substitutability) is not part of an equivalence 
relation, but it seems that in many systems, equivalence implies 
substitutability. Of course, the point is that it 

Integrated: 8259816: Typo in java.util.stream package description

[Stuart Marks]

On Wed, 27 Jan 2021 03:18:09 GMT, Stuart Marks  wrote:

> Fix a typo, and change an example to use Stream.toList().

This pull request has now been integrated.

Changeset: eb923685
Author:    Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/eb923685
Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod

8259816: Typo in java.util.stream package description

Reviewed-by: iris, lancea, naoto

-

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

RFR: 8259816: Typo in java.util.stream package description

[Stuart Marks]

Fix a typo, and change an example to use Stream.toList().

-

Commit messages:
 - 8259816: Typo in java.util.stream package description

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

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

Integrated: 8246788: ZoneRules invariants can be broken

[Stuart Marks]

On Fri, 22 Jan 2021 05:39:55 GMT, Stuart Marks  wrote:

> Tighten up argument checking in constructor.

This pull request has now been integrated.

Changeset: a8871776
Author:    Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/a8871776
Stats: 90 lines in 2 files changed: 87 ins; 0 del; 3 mod

8246788: ZoneRules invariants can be broken

Reviewed-by: rriggs, naoto

-

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

Re: RFR: 8246788: ZoneRules invariants can be broken

[Stuart Marks]

On Fri, 22 Jan 2021 17:12:34 GMT, Daniel Fuchs  wrote:

>> Or even maybe `rulesArray = 
>> lastRules.toArray(ZoneOffsetTransitionRule[]::new);`?
>
> Good point - but that would be:
> 
> ZoneOffsetTransitionRule[] rulesArray = 
> lastRules.toArray(ZoneOffsetTransitionRule[]::new).clone();

Interesting. This last one is more concise, but it's a bit harder to reason 
about. The lastRules implementation could return an array of a type other than 
ZOTR[]. If it's unrelated or a supertype, this would result in 
ClassCastException -- probably not a problem. If it were a subtype of ZOTR[], 
this would get stored in the object's field. Is this a problem? Turns out it 
can't happen, since ZOTR is final. While not wrong, I don't think this is the 
right idiom.

It occurs to me that there should by another overload Arrays.copyOf(array, 
newType) that changes the type without changing the length. This would let us 
get rid of the local variable.

-

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

RFR: 8246788: ZoneRules invariants can be broken

[Stuart Marks]

Tighten up argument checking in constructor.

-

Commit messages:
 - 8246788: ZoneRules invariants can be broken

Changes: https://git.openjdk.java.net/jdk/pull/2191/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2191=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8246788
  Stats: 90 lines in 2 files changed: 87 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2191.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2191/head:pull/2191

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

Re: RFR: 8259622: TreeMap.computeIfAbsent deviates from spec

[Stuart Marks]

On Wed, 13 Jan 2021 09:51:01 GMT, Tagir F. Valeev  wrote:

> Handle TreeMap::computeIfAbsent when previous mapping with null value existed 
> (in this case spec requires to overwrite the existing mapping)

Marked as reviewed by smarks (Reviewer).

-

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

Re: RFR: 8259622: TreeMap.computeIfAbsent deviates from spec

[Stuart Marks]

On Thu, 14 Jan 2021 23:25:22 GMT, Stuart Marks  wrote:

>> Handle TreeMap::computeIfAbsent when previous mapping with null value 
>> existed (in this case spec requires to overwrite the existing mapping)
>
> Marked as reviewed by smarks (Reviewer).

Thanks for picking this up. Changes look good. Please update copyright years if 
you get a chance.

-

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

Re: TreeMap in JDK15 deviates from spec

[Stuart Marks]

Thanks for filing this!

Tagir Valeev picked this up and has filed a PR:

https://git.openjdk.java.net/jdk/pull/2058

s'marks


On 1/11/21 10:05 AM, Phil Smith wrote:

Hello, I submitted bug report 9068554 recently, but I consider this a
pretty big issue and I'd like for this not to get lost in triage.

With JDK-8176894, a specialized implementation for computeIfAbsent was
added to TreeMap, however it doesn't handle null values correctly.
Spec states a mapping to null is equivalent to no mapping being
present, however code will `return t.value;` without checking that
value. The interface default, for reference, looks like `if ((v =
get(key)) == null)`.

A simple repro follows

TreeMap treemap = new TreeMap();
treemap.put("a", null);
System.out.println(treemap.computeIfAbsent("a", key -> "default"));
System.out.println(treemap.get("a"));

HashMap hashmap = new HashMap();
hashmap.put("a", null);
System.out.println(hashmap.computeIfAbsent("a", key -> "default"));
System.out.println(hashmap.get("a"));

Phil

[jdk16] Integrated: 8259298: broken link in Stream::toList spec

[Stuart Marks]

On Mon, 11 Jan 2021 23:15:07 GMT, Stuart Marks  wrote:

> Just fixing a broken link.

This pull request has now been integrated.

Changeset: 8a81cf15
Author:    Stuart Marks 
URL:   https://git.openjdk.java.net/jdk16/commit/8a81cf15
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod

8259298: broken link in Stream::toList spec

Reviewed-by: bchristi, iris, lancea, naoto

-

PR: https://git.openjdk.java.net/jdk16/pull/106

[jdk16] RFR: 8259298: broken link in Stream::toList spec

[Stuart Marks]

Just fixing a broken link.

-

Commit messages:
 - 8259298: broken link in Stream::toList spec

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

PR: https://git.openjdk.java.net/jdk16/pull/106

Re: RFR: 8258588: MD5 MessageDigest in java.util.UUID should be cached

[Stuart Marks]

On Fri, 18 Dec 2020 19:04:36 GMT, Sean Mullan  wrote:

>>> MD5 and DES were removed as SE requirements in JDK 14. See 
>>> https://bugs.openjdk.java.net/browse/JDK-8214483 for more information. 
>>> However, there are no plans to remove the implementations from the JDK at 
>>> this time.
>> 
>> In this case, should a bug report be filled to require specifying behaviour 
>> for `UUID#nameUUIDFromBytes(byte[])` in case of MD5 not being present?
>
>> > MD5 and DES were removed as SE requirements in JDK 14. See 
>> > https://bugs.openjdk.java.net/browse/JDK-8214483 for more information. 
>> > However, there are no plans to remove the implementations from the JDK at 
>> > this time.
>> 
>> In this case, should a bug report be filled to require specifying behaviour 
>> for `UUID#nameUUIDFromBytes(byte[])` in case of MD5 not being present?
> 
> Perhaps it should throw `UnsupportedOperationException`, but I'll defer to 
> someone who works in this area. It may also be useful to support type 5 UUIDs 
> which use SHA-1.

I have to say that introducing a ThreadLocal here seems like a step in the 
wrong direction. With a ThreadLocal, if I read this correctly, a MessageDigest 
will be cached with each thread that ever calls this API, and it won't be 
garbage collected until the thread dies. Some threads live indefinitely, e.g., 
the ones in the fork-join common pool. Is this what we want? Is UUID creation 
so frequent that each thread needs its own, or could thread safety be handled 
using a simple locking protocol?

I'd like to see a demonstration that fetching and allocating an MD5 
MessageDigest instance on each call is indeed a performance problem (either 
because it's too slow, or because it takes too much memory). Based on that we 
might or might not want to do some optimization of some form or another. Or we 
might want to optimize creation in MessageDigest instead of caching instances 
in UUID. More fundamentally, I'd like to see an answer to the question, "What 
problem is this change intended to solve?"

-

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

Re: Has it been considered to add inverse methods to collections which are in Optional?

[Stuart Marks]

On 12/8/20 5:30 AM, Remi Forax wrote:

De: "Dave Franken" 

Adding methods like notContains or notFilter have been considered several times 
in the past and rejected because it's too many methods with no real gain. I'm 
sure you can dig some old emails on lambda-dev and see the first iteration of 
the Stream API, it was full of convenient methods like that.
I'm sure Stuart or Brian have a template answer for that :)


Well I don't have a template handy but I can explain some of the reasoning. :-)

In general we try to avoid adding to many pure "convenience" methods that do things 
that are easily expressed other ways, e.g., through composition of existing methods 
or Java language constructs.


There are many boolean-returning methods in the JDK and we've tried to avoid 
defining methods that are simply inverses of each other. The notable exceptions are 
Objects.isNull and Objects.nonNull, which seemed sensible at the time, since 
references being null or non-null is quite fundamental to Java programming. (Note 
that these are mostly useful only as method references.)


Optional was introduced with isPresent, but without an inverse. Since Optional is 
(mostly) about having or not-having something, we later somewhat reluctantly added 
isEmpty, on the basis that the empty/present state of an Optional is as fundamental 
as null/non-null references.


Another common place where inverse methods have been requested is String. We've held 
off doing that. String has isEmpty and isBlank, which have different semantics, so 
this would naturally lead to having nonEmpty and nonBlank. There are also several 
other boolean-returning methods on String, so you can start to see how this could 
lead toward excessive method proliferation.



It would improve readability where we have the negation of isEmpty(), e.g.:

if (!myList.isEmpty()) {
}


You might or might not like this syntax, but if you don't, adding new APIs here and 
there won't really solve anything.


Personally I think adding a space after ! makes the negation stand out a bit more 
(and I'm used to this) so it doesn't bother me:


if (! myList.isEmpty()) { ...

One place where negation of a predicate does have a fairly high cost is with method 
references. If I have a stream where I want to filter for empty collections I could 
write


filter(Collection::isEmpty)

which is fairly nice, but if I want to filter for non-empty collections, I have to 
convert it to a lambda:


filter(collection -> ! collection.isEmpty())

This is mitigated by the addition of a Predicate.not method, which when statically 
imported, allows rewriting thus:


filter(not(Collection::isEmpty))

This enables using method references in a bunch of places where they couldn't be 
used before, relieving us of the need think of a name for the lambda parameter.
In particular, this can be used with the above-mentioned String::isBlank and 
String::isEmpty. The main point is that Predicate.not can be used with *any* 
predicate, which lets us avoid adding inverse methods in a bunch of different places.


s'marks

[jdk16] Integrated: 8258259: Unicode linebreak matching behavior is incorrect; backout JDK-8235812

[Stuart Marks]

On Thu, 17 Dec 2020 01:01:38 GMT, Stuart Marks  wrote:

> Back out code changes from JDK-8235812 to restore correct matching behavior. 
> Adjust test to comment out cases that tested for incorrect behavior.

This pull request has now been integrated.

Changeset: cbc3feeb
Author:Stuart Marks 
URL:   https://git.openjdk.java.net/jdk16/commit/cbc3feeb
Stats: 85 lines in 2 files changed: 8 ins; 56 del; 21 mod

8258259: Unicode linebreak matching behavior is incorrect; backout JDK-8235812

Reviewed-by: naoto

-

PR: https://git.openjdk.java.net/jdk16/pull/40

Re: RFR: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array [v4]

[Stuart Marks]

On Wed, 16 Dec 2020 17:16:14 GMT, Brian Burkhalter  wrote:

>> Please review this small verbiage change to specify clearly the behavior of 
>> `Reader::read(char[] cbuf)` when the length of `cbuf` is zero, and that of 
>> `Reader::read(char[] cbuf, int off, int len)` when `len` is zero.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8248383: Convert test ReadIntoZeroLengthArray to TestNG

Marked as reviewed by smarks (Reviewer).

-

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

[jdk16] RFR: 8258259: Unicode linebreak matching behavior is incorrect; backout JDK-8235812

[Stuart Marks]

Back out code changes from JDK-8235812 to restore correct matching behavior. 
Adjust test to comment out cases that tested for incorrect behavior.

-

Commit messages:
 - 8258259: Unicode linebreak matching behavior is incorrect; backout 
JDK-8235812

Changes: https://git.openjdk.java.net/jdk16/pull/40/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk16=40=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8258259
  Stats: 85 lines in 2 files changed: 8 ins; 56 del; 21 mod
  Patch: https://git.openjdk.java.net/jdk16/pull/40.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/40/head:pull/40

PR: https://git.openjdk.java.net/jdk16/pull/40

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]

[Stuart Marks]

On Wed, 9 Dec 2020 00:32:44 GMT, Paul Sandoz  wrote:

>> Stuart Marks has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fix typo, clarify asserts disabled, test prefGrowth==0
>
> Marked as reviewed by psandoz (Reviewer).

@Martin-Buchholz Any comments on this?

-

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

Re: RFR: 8253497: Core Libs Terminology Refresh [v5]

[Stuart Marks]

On Wed, 16 Dec 2020 20:08:11 GMT, Brent Christian  wrote:

>> This is part of an effort in the JDK to replace archaic/non-inclusive words 
>> with more neutral terms (see JDK-8253315 for details).
>> 
>> Here are the changes covering core libraries code and tests.  Terms were 
>> changed as follows:
>> 1. grandfathered -> legacy
>> 2. blacklist -> filter or reject
>> 3. whitelist -> allow or accept
>> 4. master -> coordinator
>> 5. slave -> worker
>> 
>> Addressing similar issues in upstream 3rd party code is out of scope of this 
>> PR.  Such changes will be picked up from their upstream sources.
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Use quotes instead of @code in Locale

Marked as reviewed by smarks (Reviewer).

-

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

Re: RFR: 8253497: Core Libs Terminology Refresh [v4]

[Stuart Marks]

On Tue, 15 Dec 2020 23:14:14 GMT, Brent Christian  wrote:

>> This is part of an effort in the JDK to replace archaic/non-inclusive words 
>> with more neutral terms (see JDK-8253315 for details).
>> 
>> Here are the changes covering core libraries code and tests.  Terms were 
>> changed as follows:
>> 1. grandfathered -> legacy
>> 2. blacklist -> filter or reject
>> 3. whitelist -> allow or accept
>> 4. master -> coordinator
>> 5. slave -> worker
>> 
>> Addressing similar issues in upstream 3rd party code is out of scope of this 
>> PR.  Such changes will be picked up from their upstream sources.
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   improve SERIAL_FILTER_PATTERN comment

Marked as reviewed by smarks (Reviewer).

-

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

Re: RFR: 8248383: Clarify java.io.Reader.read(char[], ...) behavior for full array [v3]

[Stuart Marks]

On Thu, 10 Dec 2020 23:36:13 GMT, Brian Burkhalter  wrote:

>> Please review this small verbiage change to specify clearly the behavior of 
>> `Reader::read(char[] cbuf)` when the length of `cbuf` is zero, and that of 
>> `Reader::read(char[] cbuf, int off, int len)` when `len` is zero.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8248383: Re-indent subclasses in @see tages

Marked as reviewed by smarks (Reviewer).

-

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

Re: RFR: 8253497: Core Libs Terminology Refresh [v2]

[Stuart Marks]

On Tue, 15 Dec 2020 09:17:03 GMT, Magnus Ihse Bursie  wrote:

>> Your call, I'm not a native English speaker :-)  It felt to me it's 
>> 'restrictive' than 'restrictively', an adj placed after the noun, e.g. a 
>> restrictive allow-list.
>
> It's an adverb, since it's the act of 'defining' that is being done too 
> restrictively or broadly. If you want to have an adjective you would need to 
> rephrase it so it applies to the noun, like 'defining a too restrictive 
> accept-list'. My non-native English vibes would still prefer the former.

I'd suggest `... as defining an allow-list that is too narrow or a reject-list 
that is too-wide may prevent ...`.

-

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

Re: RFR: 8258422: Cleanup unnecessary null comparison before instanceof check in java.base

[Stuart Marks]

On Sun, 4 Oct 2020 11:55:50 GMT, Andrey Turbanov 
 wrote:

>> 8258422: Cleanup unnecessary null comparison before instanceof check in 
>> java.base
>
> I believe this changes is useful and still actual:
> 1. improve code to make it easier to read.
> 2. performance should be improved a bit too

I’ll see if I can get somebody to take a look at this.

-

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

Re: RFR: JDK-8257086: Clarify differences between {Float, Double}.equals and ==

[Stuart Marks]

On Tue, 8 Dec 2020 22:12:29 GMT, Joe Darcy  wrote:

>> I'll note initially that the original bug is about `equals` and `==` whereas 
>> this change also covers `compareTo` and additional comparison operators `<` 
>> and `>`. I believe covering this additional material **IS** important, as 
>> these concepts are all closely related.
>> 
>> While this material is covering the right ground, I'd say that it's too 
>> verbose for a method specification. It feels like it's being compressed to 
>> fit into a method specification and thus doesn't do the topic justice.
>> 
>> (One additional concept that ought to be covered is that `compareTo` is 
>> *consistent with equals*. This should be asserted in the method 
>> specification, but trying to explain it in a method specification seems 
>> difficult.)
>> 
>> I'll suggest a couple alternatives. One is to put a full, combined 
>> explanation into class doc somewhere, say in `Double`. The various methods 
>> can then make some terse assertions and then refer to the combined 
>> explanation. `Float` could refer to `Double` instead of replicating the text.
>> 
>> Another alternative is to put this explanation into the `java.lang` package 
>> specification. This might be a good fit, since there is already some 
>> explanation there of the boxed primitives.
>
>> 
>> 
>> I'll note initially that the original bug is about `equals` and `==` whereas 
>> this change also covers `compareTo` and additional comparison operators `<` 
>> and `>`. I believe covering this additional material **IS** important, as 
>> these concepts are all closely related.
>> 
>> While this material is covering the right ground, I'd say that it's too 
>> verbose for a method specification. It feels like it's being compressed to 
>> fit into a method specification and thus doesn't do the topic justice.
>> 
>> (One additional concept that ought to be covered is that `compareTo` is 
>> _consistent with equals_. This should be asserted in the method 
>> specification, but trying to explain it in a method specification seems 
>> difficult.)
>> 
>> I'll suggest a couple alternatives. One is to put a full, combined 
>> explanation into class doc somewhere, say in `Double`. The various methods 
>> can then make some terse assertions and then refer to the combined 
>> explanation. `Float` could refer to `Double` instead of replicating the text.
>> 
>> Another alternative is to put this explanation into the `java.lang` package 
>> specification. This might be a good fit, since there is already some 
>> explanation there of the boxed primitives.
> 
> 
> I added discussion of compareTo as well as equals to the scope of the bug 
> since they are sibling surprising deviations from what is expected and have 
> the same root cause. (I took care to say "incomplete order" rather than 
> "partial order" since a partial order requires reflexivity.)
> 
> The interface java.lang.Comparable gives a definition of "consistent with 
> equals." I didn't verify it doesn't have an anchor to link to, but we don't 
> typically link to the definition. However, I wouldn't be adverse to having 
> "consistent with equals" link to Comparable; that would be more obvious than 
> expecting the reader to follow the "Specified by:  compareTo in interface 
> Comparable" link javadoc generates for the method.
> 
> I think the Float and Double equals and compareTo methods should at least get 
> pointers to any new section added -- I think a standalone section in the 
> package javadoc by itself would have low discoverability.
> 
> I'm make another iteration over the text; thanks.

Great, let me know if you'd like me to provide some text for any particular 
topics in this area.

-

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

Re: RFR: JDK-8257086: Clarify differences between {Float, Double}.equals and ==

[Stuart Marks]

On Tue, 8 Dec 2020 16:29:49 GMT, Joe Darcy  wrote:

> Updates to the specifications of Double.{equals, compareTo} to explain more 
> explicitly why the obvious wrappers around "==" and "<"/"==" are not 
> sufficient for floating-point values.
> 
> Once the wording is worked out, I'll replicate it for the analogous methods 
> on Float.

I'll note initially that the original bug is about `equals` and `==` whereas 
this change also covers `compareTo` and additional comparison operators `<` and 
`>`. I believe covering this additional material **IS** important, as these 
concepts are all closely related.

While this material is covering the right ground, I'd say that it's too verbose 
for a method specification. It feels like it's being compressed to fit into a 
method specification and thus doesn't do the topic justice.

(One additional concept that ought to be covered is that `compareTo` is 
*consistent with equals*. This should be asserted in the method specification, 
but trying to explain it in a method specification seems difficult.)

I'll suggest a couple alternatives. One is to put a full, combined explanation 
into class doc somewhere, say in `Double`. The various methods can then make 
some terse assertions and then refer to the combined explanation. `Float` could 
refer to `Double` instead of replicating the text.

Another alternative is to put this explanation into the `java.lang` package 
specification. This might be a good fit, since there is already some 
explanation there of the boxed primitives.

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]

[Stuart Marks]

> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
> exception message, and adds a test. In addition to some renaming and a bit of 
> refactoring of the actual code, I also made two changes of substance to the 
> code:
> 
> 1. I fixed a problem with overflow checking. In the original code, if 
> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
> method could return a negative value. It turns out that writing tests helps 
> find bugs!
> 
> 2. Under the old policy, if oldLength and minGrowth required a length above 
> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
> allocate an array of that length will almost certainly cause the Hotspot to 
> throw OOME because its implementation limit was exceeded. Instead, if the 
> required length is in this range, this method returns that required length.
> 
> Separately, I'll work on retrofitting various call sites around the JDK to 
> use this method.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  fix typo, clarify asserts disabled, test prefGrowth==0

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1617/files
  - new: https://git.openjdk.java.net/jdk/pull/1617/files/03b7922f..bacb5f91

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

  Stats: 3 lines in 2 files changed: 2 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1617.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1617/head:pull/1617

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

Re: MatchResult support for named groups

[Stuart Marks]

OK, thanks, good information. Clearly support for named groups is the most important 
thing missing from MatchResult.


s'marks

On 12/4/20 11:43 PM, Cay Horstmann wrote:

Hi Stuart,

1: If there is no match at all, then results yields the empty stream. I don't think 
anything else is required.


2/3: I wrote a fair number of regex patterns in Java, ever since they appeared in 
2002. I can say with confidence that I never once used hitEnd/requireEnd, or seen it 
used. I note they occur in one file in the JDK 11 source, in the Scanner class. But 
not in a loop that fetches all matches, but after a single call to find or 
lookingAt. I think these are exceedingly uncommon.


In contrast, looping over matcher.find() and extracting groups is common, and named 
groups are a best practice 
(https://urldefense.com/v3/__https://eslint.org/docs/rules/prefer-named-capture-group__;!!GqivPVa7Brio!I3YBH6KonWfqm4zW7pQRatPsLcj4rRjGOveB6NWQedZVU8BeJ3hknZcPy7rC1G2fug$ 
).


Cheers,

Cay

Il 04/12/2020 19:53, Stuart Marks ha scritto:

Hi Cay,

Thanks for mentioning this. It's good to know that adding this provides value to 
people who are actually trying to use this stuff (as opposed to adding stuff 
merely for the sake of completeness, as often seems to arise).


I've added some notes to JDK-8065554.

Looking at this more closely, it seems to me that MatchResult ought to include 
more match-result-related information that's currently only in Matcher, namely:


1. whether there was a match at all
2. hitEnd
3. requireEnd

If you have any thoughts on these, please let me know.

s'marks

On 12/2/20 2:53 AM, Cay Horstmann wrote:

Hello, I'd like to raise awareness for

https://bugs.openjdk.java.net/browse/JDK-8180352
https://bugs.openjdk.java.net/browse/JDK-8072984
https://bugs.openjdk.java.net/browse/JDK-8065554

These all ask for MatchResult.group(String name). What they don't mention is that 
this is more urgent in light of the methods


Stream Matcher.results() // 
https://bugs.openjdk.java.net/browse/JDK-8071479
Stream Scanner.findAll(Pattern pattern) // 
https://bugs.openjdk.java.net/browse/JDK-8072722


In particular, Matcher.results() seems a cleaner way of collecting match results 
than calling while (matcher.find()).


But then MatchResult needs to support the same queries that Matcher provides. I 
believe the only missing one is group(String name).


Cheers,

Cay

NB. There are related requests that ask for finding group names in patterns, or 
for correlating group names and numbers. I have formed no opinion on their merits.

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Mon, 7 Dec 2020 16:05:11 GMT, Roger Riggs  wrote:

>> The origin of this code is in collections like ArrayList that have an 
>> existing array (hence oldLength >= 0) and that need it to grow (hence 
>> minGrowth > 0). Those were the prevailing assumptions in the code from which 
>> this was derived, so they turn into preconditions here. I don't see the need 
>> to try to make this handle any more cases than it currently does. Doing so 
>> complicates the analysis and possibly the code as well. Certainly a bug in a 
>> caller might be difficult to track down, but I don't want to add argument 
>> checking or to provide "reasonable" behavior in the face of unreasonable 
>> inputs. This is an internal method; bugs in callers should be fixed.
>
> The algorithm can be well defined for minGrowth and prefGrowth == 0 without 
> extra checks or exceptions with a careful look at the inequality checks.
> For example, as currently coded, if both are zero, it returns 
> SOFT_MAX_ARRAY_LENGTH. 
> Changing the `0 < prefLength` to `0 <= prefLength` would return minGrowth and 
> avoid a very large but unintentional allocation.

That's only true if oldLength is zero. The behavior is different if oldLength 
is positive. In any case, this method is called when the array needs to 
**grow**, which means the caller needs to reallocate and copy. If the caller 
passes zero for both minGrowth and prefGrowth, the caller doesn't need to 
reallocate and copy, and there's no point in it calling this method in the 
first place.

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Fri, 4 Dec 2020 16:02:33 GMT, Roger Riggs  wrote:

>> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
>> exception message, and adds a test. In addition to some renaming and a bit 
>> of refactoring of the actual code, I also made two changes of substance to 
>> the code:
>> 
>> 1. I fixed a problem with overflow checking. In the original code, if 
>> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
>> method could return a negative value. It turns out that writing tests helps 
>> find bugs!
>> 
>> 2. Under the old policy, if oldLength and minGrowth required a length above 
>> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
>> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
>> allocate an array of that length will almost certainly cause the Hotspot to 
>> throw OOME because its implementation limit was exceeded. Instead, if the 
>> required length is in this range, this method returns that required length.
>> 
>> Separately, I'll work on retrofitting various call sites around the JDK to 
>> use this method.
>
> Nice clean description of the algorithm.

For what it's worth, the following code:

var list = Collections.nCopies(Integer.MAX_VALUE - 3, "");
var list2 = new ArrayList<>(list);
list2.addAll(list);

results in `java.lang.NegativeArraySizeException: -8` where the -8 is returned 
by `ArraysSupport.newLength`. This is a demonstration of the problem with 
overflow checking that is fixed by this change.

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Fri, 4 Dec 2020 19:13:45 GMT, Roger Riggs  wrote:

>> The preconditions aren't checked, because this is an internal method, and 
>> the code size is minimized in order to help inlining. That's also why 
>> `hugeLength` is a separate method. (I guess I could add comments to this 
>> effect.) With this in mind it's hard to reason about robustness. If 
>> prefLength is zero, this can only be because some precondition was violated 
>> (e.g., oldLength is negative). If this were to occur there doesn't seem to 
>> be any advantage choosing one undefined behavior over another.
>
> Is there a reason the code would not naturally work when either min or 
> preferred is zero?
> Why are their preconditions?  What do they allow?
> These methods are used in enough places that a slip in any of the clients 
> would be trouble and hard to track down.

The origin of this code is in collections like ArrayList that have an existing 
array (hence oldLength >= 0) and that need it to grow (hence minGrowth > 0). 
Those were the prevailing assumptions in the code from which this was derived, 
so they turn into preconditions here. I don't see the need to try to make this 
handle any more cases than it currently does. Doing so complicates the analysis 
and possibly the code as well. Certainly a bug in a caller might be difficult 
to track down, but I don't want to add argument checking or to provide 
"reasonable" behavior in the face of unreasonable inputs. This is an internal 
method; bugs in callers should be fixed.

-

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

Re: MatchResult support for named groups

[Stuart Marks]

Hi Cay,

Thanks for mentioning this. It's good to know that adding this provides value to 
people who are actually trying to use this stuff (as opposed to adding stuff merely 
for the sake of completeness, as often seems to arise).


I've added some notes to JDK-8065554.

Looking at this more closely, it seems to me that MatchResult ought to include more 
match-result-related information that's currently only in Matcher, namely:


1. whether there was a match at all
2. hitEnd
3. requireEnd

If you have any thoughts on these, please let me know.

s'marks

On 12/2/20 2:53 AM, Cay Horstmann wrote:

Hello, I'd like to raise awareness for

https://bugs.openjdk.java.net/browse/JDK-8180352
https://bugs.openjdk.java.net/browse/JDK-8072984
https://bugs.openjdk.java.net/browse/JDK-8065554

These all ask for MatchResult.group(String name). What they don't mention is that 
this is more urgent in light of the methods


Stream Matcher.results() // 
https://bugs.openjdk.java.net/browse/JDK-8071479
Stream Scanner.findAll(Pattern pattern) // 
https://bugs.openjdk.java.net/browse/JDK-8072722


In particular, Matcher.results() seems a cleaner way of collecting match results 
than calling while (matcher.find()).


But then MatchResult needs to support the same queries that Matcher provides. I 
believe the only missing one is group(String name).


Cheers,

Cay

NB. There are related requests that ask for finding group names in patterns, or for 
correlating group names and numbers. I have formed no opinion on their merits.

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Fri, 4 Dec 2020 15:53:01 GMT, Roger Riggs  wrote:

>> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
>> exception message, and adds a test. In addition to some renaming and a bit 
>> of refactoring of the actual code, I also made two changes of substance to 
>> the code:
>> 
>> 1. I fixed a problem with overflow checking. In the original code, if 
>> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
>> method could return a negative value. It turns out that writing tests helps 
>> find bugs!
>> 
>> 2. Under the old policy, if oldLength and minGrowth required a length above 
>> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
>> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
>> allocate an array of that length will almost certainly cause the Hotspot to 
>> throw OOME because its implementation limit was exceeded. Instead, if the 
>> required length is in this range, this method returns that required length.
>> 
>> Separately, I'll work on retrofitting various call sites around the JDK to 
>> use this method.
>
> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 640:
> 
>> 638: int prefLength = oldLength + Math.max(minGrowth, prefGrowth); 
>> // might overflow
>> 639: if (0 < prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
>> 640: return prefLength;
> 
> In spite of the assert `minGrowth > 0`, that is unchecked, I would suggest 
> prefLength == 0 to return prefLength.
> ```if (0 <= prefLength && prefLength <= SOFT_MAX_ARRAY_LENGTH) {
> return prefLength;```
> Otherwise, it falls into hughLength(...) which will return the 
> SOFT_MAX_ARRAY_LENGTH.
> 
> It would be more robust if the algorithm was well defined if either min or 
> pref were zero.

The preconditions aren't checked, because this is an internal method, and the 
code size is minimized in order to help inlining. That's also why `hugeLength` 
is a separate method. (I guess I could add comments to this effect.) With this 
in mind it's hard to reason about robustness. If prefLength is zero, this can 
only be because some precondition was violated (e.g., oldLength is negative). 
If this were to occur there doesn't seem to be any advantage choosing one 
undefined behavior over another.

> test/jdk/jdk/internal/util/ArraysSupport/NewLength.java line 70:
> 
>> 68: { IMAX-2, 1,  IMAX,   IMAX-1 },
>> 69: { IMAX-1, 1,  IMAX,   IMAX   }
>> 70: };
> 
> Adding test cases for zero (0) for the min and preferred would be good to 
> include and show any unpredictable behavior.

No, I don't want to add test cases that violate the preconditions. I guess I 
could add a prefGrowth==0 case though.

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

On Fri, 4 Dec 2020 15:47:51 GMT, Roger Riggs  wrote:

>> This rewrites the doc of ArraysSupport.newLength, adds detail to the 
>> exception message, and adds a test. In addition to some renaming and a bit 
>> of refactoring of the actual code, I also made two changes of substance to 
>> the code:
>> 
>> 1. I fixed a problem with overflow checking. In the original code, if 
>> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this 
>> method could return a negative value. It turns out that writing tests helps 
>> find bugs!
>> 
>> 2. Under the old policy, if oldLength and minGrowth required a length above 
>> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would 
>> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to 
>> allocate an array of that length will almost certainly cause the Hotspot to 
>> throw OOME because its implementation limit was exceeded. Instead, if the 
>> required length is in this range, this method returns that required length.
>> 
>> Separately, I'll work on retrofitting various call sites around the JDK to 
>> use this method.
>
> src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 654:
> 
>> 652: return SOFT_MAX_ARRAY_LENGTH;
>> 653: } else {
>> 654: return minLength;
> 
> Isn't this last `else if... then.. else` the same as:
> `return Math.max(minLength, SOFT_MAX_ARRAY_LENGTH)`

It is, and I considered replacing it, but I felt that it obscured what was 
going on.

-

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

Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

Hi Martin,

I'd appreciate it if you could take a look at this.

Thanks,

s'marks


On 12/3/20 10:57 PM, Stuart Marks wrote:

This rewrites the doc of ArraysSupport.newLength, adds detail to the exception 
message, and adds a test. In addition to some renaming and a bit of refactoring 
of the actual code, I also made two changes of substance to the code:

1. I fixed a problem with overflow checking. In the original code, if oldLength 
and prefGrowth were both very large (say, Integer.MAX_VALUE), this method could 
return a negative value. It turns out that writing tests helps find bugs!

2. Under the old policy, if oldLength and minGrowth required a length above 
SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would return 
Integer.MAX_VALUE. That doesn't make any sense, because attempting to allocate 
an array of that length will almost certainly cause the Hotspot to throw OOME 
because its implementation limit was exceeded. Instead, if the required length 
is in this range, this method returns that required length.

Separately, I'll work on retrofitting various call sites around the JDK to use 
this method.

-

Commit messages:
  - 8247373: ArraysSupport.newLength doc, test, and exception message

Changes: https://git.openjdk.java.net/jdk/pull/1617/files
  Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1617=00
   Issue: https://bugs.openjdk.java.net/browse/JDK-8247373
   Stats: 171 lines in 4 files changed: 137 ins; 3 del; 31 mod
   Patch: https://git.openjdk.java.net/jdk/pull/1617.diff
   Fetch: git fetch https://git.openjdk.java.net/jdk pull/1617/head:pull/1617

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

RFR: 8247373: ArraysSupport.newLength doc, test, and exception message

[Stuart Marks]

This rewrites the doc of ArraysSupport.newLength, adds detail to the exception 
message, and adds a test. In addition to some renaming and a bit of refactoring 
of the actual code, I also made two changes of substance to the code:

1. I fixed a problem with overflow checking. In the original code, if oldLength 
and prefGrowth were both very large (say, Integer.MAX_VALUE), this method could 
return a negative value. It turns out that writing tests helps find bugs!

2. Under the old policy, if oldLength and minGrowth required a length above 
SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would return 
Integer.MAX_VALUE. That doesn't make any sense, because attempting to allocate 
an array of that length will almost certainly cause the Hotspot to throw OOME 
because its implementation limit was exceeded. Instead, if the required length 
is in this range, this method returns that required length.

Separately, I'll work on retrofitting various call sites around the JDK to use 
this method.

-

Commit messages:
 - 8247373: ArraysSupport.newLength doc, test, and exception message

Changes: https://git.openjdk.java.net/jdk/pull/1617/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1617=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8247373
  Stats: 171 lines in 4 files changed: 137 ins; 3 del; 31 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1617.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1617/head:pull/1617

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

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements

[Stuart Marks]

On Sat, 28 Nov 2020 08:42:52 GMT, John Lin 
 wrote:

>> @johnlinp In any case, you'd also need to update the surrounding prose; we 
>> need to remove this:
>>  returning the current value or {@code null} if absent:```
>
> @pavelrappo Please see my updated CSR below. Thanks.
> 
> # Map::compute should have the implementation requirement match its default 
> implementation
> 
> ## Summary
> 
> The implementation requirement of Map::compute does not match its default 
> implementation. Besides, it has some other minor issues. We should fix it.
> 
> ## Problem
> 
> The documentation of the implementation requirements for Map::compute has the 
> following problems:
> 1. It doesn't match its default implementation.
> 1. It lacks of the return statements for most of the if-else cases.
> 1. The indents are 3 spaces, while the convention is 4 spaces.
> 1. The if-else is overly complicated and can be simplified.
> 1. The surrounding prose contains incorrect statements.
> 
> ## Solution
> 
> Rewrite the documentation of Map::compute to match its default implementation 
> and solve the above mentioned problems.
> 
> ## Specification
> 
> diff --git a/src/java.base/share/classes/java/util/Map.java 
> b/src/java.base/share/classes/java/util/Map.java
> index b1de34b42a5..b30e3979259 100644
> --- a/src/java.base/share/classes/java/util/Map.java
> +++ b/src/java.base/share/classes/java/util/Map.java
> @@ -1107,23 +1107,17 @@ public interface Map {
>   *
>   * @implSpec
>   * The default implementation is equivalent to performing the following
> - * steps for this {@code map}, then returning the current value or
> - * {@code null} if absent:
> + * steps for this {@code map}:
>   *
>   *  {@code
>   * V oldValue = map.get(key);
>   * V newValue = remappingFunction.apply(key, oldValue);
> - * if (oldValue != null) {
> - *if (newValue != null)
> - *   map.put(key, newValue);
> - *else
> - *   map.remove(key);
> - * } else {
> - *if (newValue != null)
> - *   map.put(key, newValue);
> - *else
> - *   return null;
> + * if (newValue != null) {
> + * map.put(key, newValue);
> + * } else if (oldValue != null || map.containsKey(key)) {
> + * map.remove(key);
>   * }
> + * return newValue;
>   * }
>   *
>   * The default implementation makes no guarantees about detecting if 
> the

The current snippet proposed by @johnlinp does seem to have the same behavior 
as the default implementation; I would avoid trying to "optimize" this. 
However, it does express the conditions and return value somewhat differently 
from the way the default implementation does. I think those differences are not 
significant to subclasses and are mostly stylistic. The original `@implSpec` 
snippet attempted to handle the cases separately, whereas the current proposed 
snippet minimizes them (while still agreeing with the implementation's 
behavior). I'm not too concerned about this. I think the current snippet is 
acceptable. Again, the main priority is agreement with the implementation.

-

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

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements

[Stuart Marks]

On Mon, 30 Nov 2020 15:08:51 GMT, Pavel Rappo  wrote:

>> @johnlinp, thanks for updating the CSR draft; it is much better now.
>> 
>> @stuart-marks, I think we could further improve this snippet. This `if` 
>> statement seems to use an optimization:
>> 
>> if (oldValue != null || map.containsKey(key))
>> 
>> I don't think we should include an optimization into the specification 
>> unless that optimization also improves readability. Is this the case here? 
>> Could this be better?
>> 
>> if (map.containsKey(key))
>
> I would even go as far as to rewrite that snippet like this:
> 
> if (newValue == null) {
> remove(key);
> } else {
> put(key, newValue);
> }
> return newValue;
> 
> This rewrite is possible thanks to the following properties of 
> `Map.remove(Object key)`:
> 
> 1. A call with an unmapped `key` has no effect.
> 2. A call with a mapped `key` has the same semantics regardless of the value 
> that this key is mapped to.
> 
> In particular, (2) covers `null` values.
> 
> To me, this rewrite reads better; however, I understand that readability is 
> subjective and that snippets used in `@implSpec` might be subject to 
> additional requirements.

@pavelrappo The intended effect of the revised snippet is sensible, but again I 
note that it differs from the actual default implementation. Specifically: if 
the map does not contain the key and newValue is null, the default 
implementation currently does nothing, whereas the revised snippet calls 
`remove(key)`. This should have no effect _on the map_ but a subclass might 
override `remove` and this behavior difference is observable. (The typical 
example of this is maintaining a counter of the number of operations. I think 
_Effective Java_ uses that example in discussing subclassing.) I think the main 
priority here is fidelity to what the default implementation actually does -- 
at least, concerning operations on *this* -- and less on readability.

-

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

Integrated: 8228615: Optional.empty doc should suggest using isEmpty

[Stuart Marks]

On Thu, 3 Dec 2020 01:08:18 GMT, Stuart Marks  wrote:

> Some small doc changes. The changes are to `@apiNote` text, which is 
> non-normative, so no CSR is required.

This pull request has now been integrated.

Changeset: 2b73f992
Author:Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/2b73f992
Stats: 12 lines in 4 files changed: 0 ins; 0 del; 12 mod

8228615: Optional.empty doc should suggest using isEmpty

Reviewed-by: lancea, bpb, naoto

-

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

RFR: 8228615: Optional.empty doc should suggest using isEmpty

[Stuart Marks]

Some small doc changes. The changes are to `@apiNote` text, which is 
non-normative, so no CSR is required.

-

Commit messages:
 - 8228615: Optional.empty() doc should suggest isEmpty() instead of isPresent()

Changes: https://git.openjdk.java.net/jdk/pull/1585/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1585=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8228615
  Stats: 12 lines in 4 files changed: 0 ins; 0 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1585.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1585/head:pull/1585

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

Integrated: 8180352: Add Stream.toList() method

[Stuart Marks]

On Tue, 3 Nov 2020 01:33:32 GMT, Stuart Marks  wrote:

> This change introduces a new terminal operation on Stream. This looks like a 
> convenience method for Stream.collect(Collectors.toList()) or 
> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
> method directly on Stream enables it to do what can't easily by done by a 
> Collector. In particular, it allows the stream to deposit results directly 
> into a destination array (even in parallel) and have this array be wrapped in 
> an unmodifiable List without copying.
> 
> In the past we've kept most things from the Collections Framework as 
> implementations of Collector, not directly on Stream, whereas only 
> fundamental things (like toArray) appear directly on Stream. This is true of 
> most Collections, but it does seem that List is special. It can be a thin 
> wrapper around an array; it can handle generics better than arrays; and 
> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
> can be value-based. See John Rose's comments in the bug report:
> 
> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
> 
> This operation is null-tolerant, which matches the rest of Streams. This 
> isn't specified, though; a general statement about null handling in Streams 
> is probably warranted at some point.
> 
> Finally, this method is indeed quite convenient (if the caller can deal with 
> what this operation returns), as collecting into a List is the most common 
> stream terminal operation.

This pull request has now been integrated.

Changeset: 41dbc139
Author:Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/41dbc139
Stats: 445 lines in 8 files changed: 358 ins; 42 del; 45 mod

8180352: Add Stream.toList() method

Reviewed-by: psandoz

-

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

Re: RFR: 8180352: Add Stream.toList() method [v4]

[Stuart Marks]

On Wed, 25 Nov 2020 17:14:43 GMT, Peter Levart  wrote:

>> An alternative with similar performance would be to do a Stream.toArray() 
>> and then copy that array into new Object[] and then wrap that copy with 
>> listFromTrustedArrayNullsAllowed(). The difference would be in the 
>> serialization format of the resulting List and maybe also in the access 
>> performance of resulting List (no indirection via the unmodifiableList 
>> wrapper and different type for JIT to speculate about). So if we want the 
>> resulting List to behave exactly the same in both implementations of 
>> toList(), then this alternative might be preferable. WDYT?
>
> Such alternative might also be faster using intrisified array copying method 
> (here measuring just copying overhead):
> 
> Benchmark  (len)  Mode  Cnt   Score  Error  Units
> ToListBench.toList1   10  avgt   10  14.213 ±0.061  ns/op
> ToListBench.toList1 1000  avgt   10 541.883 ±3.845  ns/op
> ToListBench.toList1  100  avgt   10  753223.523 ± 4656.664  ns/op
> ToListBench.toList2   10  avgt   10   8.810 ±0.052  ns/op
> ToListBench.toList2 1000  avgt   10 264.748 ±0.807  ns/op
> ToListBench.toList2  100  avgt   10  349518.502 ± 3242.061  ns/op
> 
> https://gist.github.com/plevart/974b67b65210f8dd122773f481c0a603

I don't want to change the default implementation and its specification, for a 
couple reasons. First, this implementation won't be used in normal operation, 
as it's overridden by the JDK implementation. (I expect that stream extension 
libraries will be updated to override it as well.) Second, I'm quite 
uncomfortable using an internal method from within a default implementation. 
Right now this method is an agreement between the unmodifiable collections 
implementation and the ReferencePipeline implementation, and we have complete 
freedom to change this agreement at any time. If this internal method were used 
from a default implementation, its behavior would have to be specified in 
`@implSpec` somehow. Certainly one could write some very vague words for the 
specification that allow some freedom to make changes, but this has to choose 
between a better specification and more implementation freedom. I don't see the 
value in having to make this tradeoff at all.

An extra copy can be avoided via a private agreement between ArrayList and 
Arrays.asList, which should probably be done in any case.

-

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

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements

[Stuart Marks]

On Wed, 25 Nov 2020 16:39:20 GMT, Pavel Rappo  wrote:

>> @pavelrappo Please see my proposed CSR below. Thank you.
>> 
>> # Map::compute should have a clearer implementation requirement.
>> 
>> ## Summary
>> 
>> java.util.Map::compute should have a clearer implementation requirement in 
>> its documentation.
>> 
>> ## Problem
>> 
>> The documentation of the implementation requirements for Map::compute has 
>> the following problems:
>> 1. It lacks of return statements for most of the if-else cases.
>> 1. The indents are 3 spaces, while the convention is 4 spaces.
>> 1. The if-else is overly complicated and can be simplified.
>> 
>> ## Solution
>> 
>> Rewrite the documentation of Map::compute to match its default 
>> implementation.
>> 
>> ## Specification
>> 
>> diff --git a/src/java.base/share/classes/java/util/Map.java 
>> b/src/java.base/share/classes/java/util/Map.java
>> index b1de34b42a5..c3118a90581 100644
>> --- a/src/java.base/share/classes/java/util/Map.java
>> +++ b/src/java.base/share/classes/java/util/Map.java
>> @@ -1113,17 +1113,12 @@ public interface Map {
>>   *  {@code
>>   * V oldValue = map.get(key);
>>   * V newValue = remappingFunction.apply(key, oldValue);
>> - * if (oldValue != null) {
>> - *if (newValue != null)
>> - *   map.put(key, newValue);
>> - *else
>> - *   map.remove(key);
>> - * } else {
>> - *if (newValue != null)
>> - *   map.put(key, newValue);
>> - *else
>> - *   return null;
>> + * if (newValue != null) {
>> + * map.put(key, newValue);
>> + * } else if (oldValue != null) {
>> + * map.remove(key);
>>   * }
>> + * return newValue;
>>   * }
>>   *
>>   * The default implementation makes no guarantees about detecting if 
>> the
>
> @johnlinp In any case, you'd also need to update the surrounding prose; we 
> need to remove this:
>  returning the current value or {@code null} if absent:```

@pavelrappo 

> What is the required level of fidelity particular (pseudo-) code has to have?

It's potentially a large discussion, one that could be had in the context of my 
JEP draft http://openjdk.java.net/jeps/8068562 . However, speaking practically, 
it's possible to focus the discussion fairly concisely: the main responsibility 
of the `@implSpec` ("Implementation Requirements") section is to give 
implementors of subclasses enough information to decide whether to inherit the 
implementation or to override it, and if they override it, what behavior they 
can expect if they were to call `super.compute`.

In this case, a null-value-tolerating Map implementation needs to know that the 
default implementation calls `remove` in the particular case that you 
mentioned. A concurrent Map implementation will also need to know that the 
default implementation calls `get(key)` and `containsKey(key)` at different 
times, potentially leading to a race condition. Both of these inform the 
override vs. inherit decision.

-

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

Re: RFR: 8180352: Add Stream.toList() method [v2]

[Stuart Marks]

On 11/18/20 3:55 AM, Florian Weimer wrote:

I think it's also needed for an efficient null element check in
List::copyOf.  I have a hunch that with the posted implementation, it
would incorrectly produce an immutable list that contains null
elements.


(Sorry for the delay; oddly, this comment didn't make it into the PR.)

Yes, there was a bug in List::copyOf in that it would pass through a list containing 
nulls, a clear violation of its spec. I fixed this in my second-most recent commit 
on this PR.


s'marks

Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements

[Stuart Marks]

On Tue, 24 Nov 2020 02:19:34 GMT, Pavel Rappo  wrote:

> 2. Both the proposed pseudo-code and the existing pseudo-code deviate from 
> the documented behaviour (written in prose) and the actual implementation.

Let me clarify something. The "documented behavior" is actually the API 
specification, the contract that applies to Map.compute and all its 
implementations. The pseudo-code is part of the "implementation requirements" 
section, whose primary responsibility is to specify what the default 
implementation actually does. (Of course, that implementation should also 
conform to the API spec.)

We've established that the pseudo-code -- either the existing or proposed 
version -- differs from the default implementation's actual behavior. It's thus 
failed its primary responsibility, and therefore that's the problem that needs 
to be fixed. I think it's pointless to clean up the pseudo-code if the result 
is still incorrect.

-

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

Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v3]

[Stuart Marks]

On Sun, 22 Nov 2020 16:00:45 GMT, Alan Bateman  wrote:

>> This change terminally deprecates the following methods defined by 
>> java.lang.ThreadGroup 
>> 
>> - stop 
>> - destroy 
>> - isDestroyed 
>> - setDaemon 
>> - isDaemon 
>> 
>> The stop method has been deprecated since=1.2 because it is inherently 
>> unsafe. It is time to terminally deprecate this method so it can be removed 
>> in a future release. Thread.stop will be examined in a separate issue. 
>> 
>> The destroy, isDestroyed, setDaemon, isDaemon methods support the mechanism 
>> to explicitly or automatically destroy a thread group. As detailed in 
>> JDK-8252885, the mechanism to destroy thread groups is flawed and racy. 
>> Furthermore, this mechanism inhibits efforts to drop the reference from a 
>> thread group to its threads (so that thread creation, starting and 
>> termination do not need to coordinate with their thread group). These 
>> methods should be terminally deprecated so they can be degraded in a future 
>> release and eventually removed.
>> 
>> CSR with more information:  https://bugs.openjdk.java.net/browse/JDK-8256644
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains six additional 
> commits since the last revision:
> 
>  - Tweak wording of @deprecated message to make it more general
>  - Merge
>  - Fixed typo in @deprecated text
>  - Merge
>  - Update jshell class
>  - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, 
> setDaemon and isDaemon

I'm ok with the wording "inherently flawed."

-

Marked as reviewed by smarks (Reviewer).

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

Re: RFR: 8180352: Add Stream.toList() method [v4]

[Stuart Marks]

> This change introduces a new terminal operation on Stream. This looks like a 
> convenience method for Stream.collect(Collectors.toList()) or 
> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
> method directly on Stream enables it to do what can't easily by done by a 
> Collector. In particular, it allows the stream to deposit results directly 
> into a destination array (even in parallel) and have this array be wrapped in 
> an unmodifiable List without copying.
> 
> In the past we've kept most things from the Collections Framework as 
> implementations of Collector, not directly on Stream, whereas only 
> fundamental things (like toArray) appear directly on Stream. This is true of 
> most Collections, but it does seem that List is special. It can be a thin 
> wrapper around an array; it can handle generics better than arrays; and 
> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
> can be value-based. See John Rose's comments in the bug report:
> 
> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
> 
> This operation is null-tolerant, which matches the rest of Streams. This 
> isn't specified, though; a general statement about null handling in Streams 
> is probably warranted at some point.
> 
> Finally, this method is indeed quite convenient (if the caller can deal with 
> what this operation returns), as collecting into a List is the most common 
> stream terminal operation.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  Add comment and assert regarding array class; use switch expression.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1026/files
  - new: https://git.openjdk.java.net/jdk/pull/1026/files/15beacd2..bd890ae5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1026=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1026=02-03

  Stats: 20 lines in 1 file changed: 4 ins; 4 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1026.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1026/head:pull/1026

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