Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]

[Hannes Wallnöfer]

On Tue, 21 Sep 2021 15:27:12 GMT, Daniel Fuchs  wrote:

>> I just realized I commented on a test file, while the actual culprit is in 
>> `FileServerHandler.java`. But I guess it applies to all usages of this class 
>> and method.
>
> Hmm... When printing messages on the console don't we want a localized date? 
> Or are you referring to other usages of the date formatter?  Worth double 
> checking in any case.

The problem I was referring to was not about printing to the console. I hadn't 
thought about that, I agree the default locale should be used there. I was 
referring to `Last-modified` HTTP headers with a non-English date value, e.g.:

Last-modified: Di., 21 Sep. 2021 09:56:53 GMT

I think browsers will get confused by this. It probably isn't a big deal since 
I think the server doesn't implement conditional GETs (i.e. use the 
last-modified date in subsequent requests), but the HTTP spec is [quite strict 
][1] about date formats.

[1]: https://datatracker.ietf.org/doc/html/rfc2616#section-3.3.1

-

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

Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]

[Hannes Wallnöfer]

On Tue, 21 Sep 2021 14:59:18 GMT, Hannes Wallnöfer  wrote:

>> Julia Boes has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 16 commits:
>> 
>>  - Merge branch 'master' into simpleserver
>>  - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>>  - Merge branch 'master' into simpleserver
>>  - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>>  - Merge branch 'master' into simpleserver
>>  - check isHidden, isSymlink, isReadable for all path segments 
>>  - add checks for all path segments
>>  - Merge branch 'master' into componentcheck
>>  - Merge branch 'master' into simpleserver
>>  - improve output on startup
>>  - ... and 6 more: 
>> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131
>
> test/jdk/com/sun/net/httpserver/simpleserver/SecurityManagerTest.java line 
> 198:
> 
>> 196: 
>> 197: static final DateTimeFormatter HTTP_DATE_FORMATTER =
>> 198: DateTimeFormatter.ofPattern("EEE, dd MMM  HH:mm:ss v");
> 
> I think this and other usages of `DateTimeFormatter.ofPattern` need to be 
> called with `Locale.US` (or something similar) as second argument, otherwise 
> the current default locale will be used. I noticed because on my laptop the 
> `Last-modified` header contains a german date.

I just realized I commented on a test file, while the actual culprit is in 
`FileServerHandler.java`. But I guess it applies to all usages of this class 
and method.

-

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

Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]

[Hannes Wallnöfer]

On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes  wrote:

>> This change implements a simple web server that can be run on the 
>> command-line with `java -m jdk.httpserver`.
>> 
>> This is facilitated by adding an entry point for the `jdk.httpserver` 
>> module, an implementation class whose main method is run when the above 
>> command is executed. This is the first such module entry point in the JDK.
>> 
>> The server is a minimal HTTP server that serves the static files of a given 
>> directory, similar to existing alternatives on other platforms and 
>> convenient for testing, development, and debugging.
>> 
>> Additionally, a small API is introduced for programmatic creation and 
>> customization.
>> 
>> Testing: tier1-3.
>
> Julia Boes has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 16 commits:
> 
>  - Merge branch 'master' into simpleserver
>  - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>  - Merge branch 'master' into simpleserver
>  - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>  - Merge branch 'master' into simpleserver
>  - check isHidden, isSymlink, isReadable for all path segments 
>  - add checks for all path segments
>  - Merge branch 'master' into componentcheck
>  - Merge branch 'master' into simpleserver
>  - improve output on startup
>  - ... and 6 more: 
> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131

test/jdk/com/sun/net/httpserver/simpleserver/SecurityManagerTest.java line 198:

> 196: 
> 197: static final DateTimeFormatter HTTP_DATE_FORMATTER =
> 198: DateTimeFormatter.ofPattern("EEE, dd MMM  HH:mm:ss v");

I think this and other usages of `DateTimeFormatter.ofPattern` need to be 
called with `Locale.US` (or something similar) as second argument, otherwise 
the current default locale will be used. I noticed because on my laptop the 
`Last-modified` header contains a german date.

-

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

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

[Hannes Wallnöfer]

On Sun, 10 Jan 2021 17:04:19 GMT, Attila Szegedi  wrote:

>> _(NB: For this leak to occur, an application needs to be either creating and 
>> discarding linkers frequently, or creating and discarding class loaders 
>> frequently while creating Dynalink type converters for classes in them. 
>> Neither is typical usage, although they can occur in dynamic code loading 
>> environments such as OSGi.)_
>> 
>> I'll explain this one in a bit more detail. Dynalink creates and caches 
>> method handles for language-specific conversions between types. Each 
>> conversion is thus characterized by a `Class` object representing the type 
>> converted from, and a `Class` object representing the type converted to, 
>> thus they're keyed by a pair of `(Class, Class)`. Java API provides the 
>> excellent `ClassValue` class for associating values with a single class, but 
>> it lacks the – admittedly more niche – facility for associating a value with 
>> a pair of classes. I originally solved this problem using something that 
>> looks like a `ClassValue, T>>`. I say "looks like" because 
>> Dynalink has a specialized `ClassMap` class that works like `Map, 
>> T>` but it also goes to some pains to _not_ establish strong references from 
>> a class loader to either its children or to unrelated class loaders, for 
>> obvious reasons.
>> 
>> Surprisingly, the problem didn't lie in there, though. The problem was in 
>> the fact that `TypeConverterFactory` used `ClassValue` objects that were its 
>> non-static anonymous inner classes, and further the values they computed 
>> were also of non-static anonymous inner classes. Due to the way `ClassValue` 
>> internals work, this led to the `TypeConverterFactory` objects becoming 
>> anchored in a GC root of `Class.classValueMap` through the synthetic 
>> `this$0` reference chains in said inner classes… talk about non-obvious 
>> memory leaks. (I guess there's a lesson in there about not using 
>> `ClassValue` as an instance field, but there's a genuine need for them here, 
>> so…)
>> 
>> … so the first thing I did was I deconstructed all those anonymous inner 
>> classes into named static inner classes, and ended up with something that 
>> _did_ fix the memory leak (yay!) but at the same time there was a bunch of 
>> copying of constructor parameters from outer classes into the inner classes, 
>> the whole thing was very clunky with scary amounts of boilerplate. I felt 
>> there must exist a better solution.
>> 
>> And it exists! I ended up replacing the `ClassValue>` construct 
>> with a ground-up implementation of `BiClassValue`, representation of 
>> lazily computed values associated with a pair of types. This was the right 
>> abstraction the `TypeConverterFactory` code needed all along. 
>> `BiClassValue` is also not implemented as an abstract class but rather it 
>> is a final class and takes a `BiFunction, Class, T>` for 
>> computation of its values, thus there's never a risk of making it an 
>> instance-specific inner class (well, you still need to be careful with the 
>> bifunction lambda…) It also has an even better solution for avoiding strong 
>> references to child class loaders.
>> 
>> And that's why I'm not submitting here the smallest possible point fix to 
>> the memory leak, but rather a slightly larger one that architecturally fixes 
>> it the right way.
>> 
>> There are two test classes, they test subtly different scenarios. 
>> `TypeConverterFactoryMemoryLeakTest` tests that when a 
>> `TypeConverterFactory` instance becomes unreachable, all method handles it 
>> created also become eligible for collection. 
>> `TypeConverterFactoryRetentionTests` on the other hand test that the factory 
>> itself won't prevent class loaders from being collected by virtue of 
>> creating converters for them.
>
> Attila Szegedi has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rename ClassLoaderRelation to RetentionDirection; it is better 
> self-documenting this way.

Nice work as ever, Attila. Looks good to me.

-

Marked as reviewed by hannesw (Reviewer).

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

Re: RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

[Hannes Wallnöfer]

On Sun, 7 Feb 2021 15:45:00 GMT, Peter Levart  wrote:

>> Attila Szegedi has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Rename ClassLoaderRelation to RetentionDirection; it is better 
>> self-documenting this way.
>
> I think this looks good Attila.

> @plevart would you be then be okay with approving this PR? Also, @hns or 
> @sundararajana can I maybe get a review from either of you?

@szegedi I've looked at the BiClassValue code and it looks good to me. If 
that's ok for you I'll review the rest of the PR tomorrow morning.

-

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

Re: [jdk16] RFR: JDK-8247994: Localize javadoc search [v4]

[Hannes Wallnöfer]

On Thu, 17 Dec 2020 17:10:10 GMT, Jonathan Gibbons  wrote:

>> This is for JDK16, as a precursor to fixing JDK-8258002.
>> 
>> While it is good to be using localized strings in the generated output, the 
>> significance for JDK-8258002 is that the strings are now obtained from a 
>> resource file, and not hardcoded in JavaScript file itself.
>> 
>> The source file `search.js` is renamed to `search.js.template`, and (unlike 
>> other template files) is copied as-is into the generated image. The values 
>> in the template are resolved when javadoc is executed, depending on the 
>> locale in use at the time. Because of the change in the file's extension, 
>> two makefiles are updated to accommodate the new extension: one is for the 
>> "interim" javadoc used to generate the API docs; the other is for the 
>> primary javadoc in the main JDK image.
>
> Jonathan Gibbons has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review comment

Marked as reviewed by hannesw (Reviewer).

-

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

Re: [jdk16] RFR: JDK-8247994: Localize javadoc search [v3]

[Hannes Wallnöfer]

On Thu, 17 Dec 2020 16:39:17 GMT, Jonathan Gibbons  wrote:

>> This is for JDK16, as a precursor to fixing JDK-8258002.
>> 
>> While it is good to be using localized strings in the generated output, the 
>> significance for JDK-8258002 is that the strings are now obtained from a 
>> resource file, and not hardcoded in JavaScript file itself.
>> 
>> The source file `search.js` is renamed to `search.js.template`, and (unlike 
>> other template files) is copied as-is into the generated image. The values 
>> in the template are resolved when javadoc is executed, depending on the 
>> locale in use at the time. Because of the change in the file's extension, 
>> two makefiles are updated to accommodate the new extension: one is for the 
>> "interim" javadoc used to generate the API docs; the other is for the 
>> primary javadoc in the main JDK image.
>
> Jonathan Gibbons has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review comment

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/search.js.template
 line 40:

> 38: var MIN_RESULTS = 3;
> 39: var MAX_RESULTS = 500;
> 40: var UNNAMED = ">";

Oops, an additional closing angle bracket slipped there.

-

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

Re: [jdk16] RFR: JDK-8247994: Localize javadoc search

[Hannes Wallnöfer]

On Mon, 14 Dec 2020 09:34:31 GMT, Hannes Wallnöfer  wrote:

>> This is for JDK16, as a precursor to fixing JDK-8258002.
>> 
>> While it is good to be using localized strings in the generated output, the 
>> significance for JDK-8258002 is that the strings are now obtained from a 
>> resource file, and not hardcoded in JavaScript file itself.
>> 
>> The source file `search.js` is renamed to `search.js.template`, and (unlike 
>> other template files) is copied as-is into the generated image. The values 
>> in the template are resolved when javadoc is executed, depending on the 
>> locale in use at the time. Because of the change in the file's extension, 
>> two makefiles are updated to accommodate the new extension: one is for the 
>> "interim" javadoc used to generate the API docs; the other is for the 
>> primary javadoc in the main JDK image.
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/DocFile.java
>  line 256:
> 
>> 254: sb.append(resources.getText(m.group("key")));
>> 255: } catch (MissingResourceException e) {
>> 256: sb.append(m.group());
> 
> Shouldn't we propagate an error here, or issue a warning? If a key is missing 
> localized properties the value from default properties should be used, so 
> this should never happen, right?

I see the added check for "##REPLACE:" in TestSearch.java will catch that case, 
so I guess it's ok.

-

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

Re: [jdk16] RFR: JDK-8247994: Localize javadoc search

[Hannes Wallnöfer]

On Sun, 13 Dec 2020 00:19:59 GMT, Jonathan Gibbons  wrote:

> This is for JDK16, as a precursor to fixing JDK-8258002.
> 
> While it is good to be using localized strings in the generated output, the 
> significance for JDK-8258002 is that the strings are now obtained from a 
> resource file, and not hardcoded in JavaScript file itself.
> 
> The source file `search.js` is renamed to `search.js.template`, and (unlike 
> other template files) is copied as-is into the generated image. The values in 
> the template are resolved when javadoc is executed, depending on the locale 
> in use at the time. Because of the change in the file's extension, two 
> makefiles are updated to accommodate the new extension: one is for the 
> "interim" javadoc used to generate the API docs; the other is for the primary 
> javadoc in the main JDK image.

Looks good in general. `` shouldn't be localized as it is an internal 
tag (see inline comment).

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/search.js.template
 line 40:

> 38: var MIN_RESULTS = 3;
> 39: var MAX_RESULTS = 500;
> 40: var UNNAMED = "##REPLACE:doclet.search.unnamed##";

`` is not a string that is ever shown to the user, it is what is used 
by javadoc to denote the default package (see 
`toolkit.util.DocletConstants.DEFAULT_PACKAGE_NAME`). This variable shouldn't 
be localized as that would break behaviour for code in the default package (and 
show the localized string as package name, instead of no package name).

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/DocFile.java
 line 256:

> 254: sb.append(resources.getText(m.group("key")));
> 255: } catch (MissingResourceException e) {
> 256: sb.append(m.group());

Shouldn't we propagate an error here, or issue a warning? If a key is missing 
localized properties the value from default properties should be used, so this 
should never happen, right?

-

Changes requested by hannesw (Reviewer).

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

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v2]

[Hannes Wallnöfer]

On Tue, 20 Oct 2020 12:15:23 GMT, Jan Lahoda  wrote:

>> This is an update to javac and javadoc, to introduce support for Preview 
>> APIs, and generally improve javac and javadoc behavior to more closely 
>> adhere to JEP 12.
>> 
>> The notable changes are:
>> 
>>  * adding support for Preview APIs (javac until now supported primarily only 
>> preview language features, and APIs associated with preview language 
>> features). This includes:
>>  * the @PreviewFeature annotation has boolean attribute "reflective", 
>> which should be true for reflective Preview APIs, false otherwise. This 
>> replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>>  * the preview warnings for preview APIs are auto-suppressed as 
>> described in the JEP 12. E.g. the module that declares the preview API is 
>> free to use it without warnings
>>  * improving error/warning messages. Please see [1] for a list of 
>> cases/examples.
>>  * class files are only marked as preview if they are using a preview 
>> feature. [1] also shows if a class file is marked as preview or not.
>>  * the PreviewFeature annotation has been moved to jdk.internal.javac 
>> package (originally was in the jdk.internal package).
>>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in 
>> the source files. javadoc will auto-generate a note for @PreviewFeature 
>> elements, see e.g. [2] and [3] (non-reflective and reflective API, 
>> respectively). A summary of preview elements is also provided [4]. Existing 
>> uses of @preview have been updated/removed.
>>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses 
>> of Preview elements, and for declaring elements using preview language 
>> features [5].
>>  
>>  Please also see the CSR [6] for more information.
>>  
>>  [1] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>>  [2] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>>  [3] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>>  [4] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>>  [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>>  [6] https://bugs.openjdk.java.net/browse/JDK-8250769
>
> Jan Lahoda has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 35 commits:
> 
>  - Merge branch 'JDK-8250768-dev' of https://github.com/lahodaj/jdk into 
> JDK-8250768
>  - More fixing tests.
>  - Fixing tests.
>  - Using unique sections for preview warning sections, as suggested.
>  - Merge branch 'master' into JDK-8250768
>  - Reflecting review comments.
>  - Fixing tests.
>  - Various cleanup.
>  - The Preview taglet is not needed anymore.
>  - There is not jdk.internal package anymore
>  - ... and 25 more: 
> https://git.openjdk.java.net/jdk/compare/98ec4a67...be1d8651

The javadoc code changes look reasonable, and the preview bits in the generated 
documentation look good as well. 

Apart from my inline comments which all address minor issues, there are a few 
things I don't like around `PreviewListWriter`: its code replication with 
`DeprecatedListWriter, and it adds things to HtmlDocletWriter and HtmlStyle 
that I don't think are strictly necessary. However, I wouldn't expect you to 
know how we like things done in javadoc, so maybe the simplest solution would 
be for one of us to go over the javadoc bits, either as part of this pull 
request or (if you prefer to get it integrated rather sooner) as a follow-up 
task.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java
 line 3164:

> 3162: }
> 3163: 
> 3164: public Set elementFlags(Element el) {

It seems like the sole use of this method and the `ElementFlag` enum below is 
to determine whether a preview warning note should be generated for an element. 
Is there something that speaks against simplifying it to reflect that purpose, 
e.g. change its name to `hasPreviewNote` or `hasPreviewContent` and change the 
return type to `boolean`?  Of course that's unless you foresee adding more 
`ElementFlag` values in the future.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
 line 125:

> 123: import static javax.lang.model.element.ElementKind.METHOD;
> 124: import static javax.lang.model.element.ElementKind.PACKAGE;
> 125: import jdk.javadoc.internal.doclets.formats.html.markup.RawHtml;

These imports as well as HtmlAttr above aren't used.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/LinkInfoImpl.java
 line 247:

> 245:  * The member this link points to (if any).
> 246:  */
> 247: public Element whereMember;

I don't think `whereMember` is a great name (and `where` above is not a great 
name either). What about naming this `targetMember` 

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12

[Hannes Wallnöfer]

On Mon, 19 Oct 2020 14:09:51 GMT, Hannes Wallnöfer  wrote:

>> This is an update to javac and javadoc, to introduce support for Preview 
>> APIs, and generally improve javac and javadoc
>> behavior to more closely adhere to JEP 12.
>> The notable changes are:
>> 
>>  * adding support for Preview APIs (javac until now supported primarily only 
>> preview language features, and APIs
>>associated with preview language features). This includes:
>>  * the @PreviewFeature annotation has boolean attribute "reflective", 
>> which should be true for reflective Preview APIs,
>>false otherwise. This replaces the existing "essentialAPI" attribute 
>> with roughly inverted meaning.
>>  * the preview warnings for preview APIs are auto-suppressed as 
>> described in the JEP 12. E.g. the module that declares the
>>preview API is free to use it without warnings
>>  * improving error/warning messages. Please see [1] for a list of 
>> cases/examples.
>>  * class files are only marked as preview if they are using a preview 
>> feature. [1] also shows if a class file is marked as
>>preview or not.
>>  * the PreviewFeature annotation has been moved to jdk.internal.javac 
>> package (originally was in the jdk.internal package).
>>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in 
>> the source files. javadoc will auto-generate a
>>note for @PreviewFeature elements, see e.g. [2] and [3] (non-reflective 
>> and reflective API, respectively). A summary of
>>preview elements is also provided [4]. Existing uses of @preview have 
>> been updated/removed.
>>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses 
>> of Preview elements, and for declaring
>>elements using preview language features [5].
>>  
>>  Please also see the CSR [6] for more information.
>>  
>>  [1] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>>  [2] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>>  [3]
>>  
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>>  [4] 
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>>  [5]
>>  http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/ [6] 
>> https://bugs.openjdk.java.net/browse/JDK-8250769
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
>  line 2238:
> 
>> 2236: if (previewTree != null) {
>> 2237: previewDiv.add(new 
>> HtmlTree(TagName.A).put(HtmlAttr.ID, "preview")
>> 2238:   .add(new
>> RawHtml(utils.getPreviewTreeSummaryOrDetails(previewTree, false;
> 
> The `id` attribute needs to be unique within the page, so in addition to make 
> the value not a valid java identifier (as
> @jonathan-gibbons pointed out in a comment elsewhere) we need to support 
> multiple preview ids per page. One way to do
> this would be to add the element name to the id value, e.g. `preview- name>`.

Of course the element name won't do for overloaded methods and constructors... 
`Links#getAnchor(ExecutableElement)`
should be used for those.

-

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

Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12

[Hannes Wallnöfer]

On Fri, 16 Oct 2020 15:20:03 GMT, Jan Lahoda  wrote:

> This is an update to javac and javadoc, to introduce support for Preview 
> APIs, and generally improve javac and javadoc
> behavior to more closely adhere to JEP 12.
> The notable changes are:
> 
>  * adding support for Preview APIs (javac until now supported primarily only 
> preview language features, and APIs
>associated with preview language features). This includes:
>  * the @PreviewFeature annotation has boolean attribute "reflective", 
> which should be true for reflective Preview APIs,
>false otherwise. This replaces the existing "essentialAPI" attribute 
> with roughly inverted meaning.
>  * the preview warnings for preview APIs are auto-suppressed as described 
> in the JEP 12. E.g. the module that declares the
>preview API is free to use it without warnings
>  * improving error/warning messages. Please see [1] for a list of 
> cases/examples.
>  * class files are only marked as preview if they are using a preview 
> feature. [1] also shows if a class file is marked as
>preview or not.
>  * the PreviewFeature annotation has been moved to jdk.internal.javac package 
> (originally was in the jdk.internal package).
>  * Preview API in JDK's javadoc no longer needs to specify @preview tag in 
> the source files. javadoc will auto-generate a
>note for @PreviewFeature elements, see e.g. [2] and [3] (non-reflective 
> and reflective API, respectively). A summary of
>preview elements is also provided [4]. Existing uses of @preview have been 
> updated/removed.
>  * non-JDK javadoc is also enhanced to auto-generate preview notes for uses 
> of Preview elements, and for declaring
>elements using preview language features [5].
>  
>  Please also see the CSR [6] for more information.
>  
>  [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>  [2] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>  [3]
>  
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>  [4] 
> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>  [5]
>  http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/ [6] 
> https://bugs.openjdk.java.net/browse/JDK-8250769

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
 line 2238:

> 2236: if (previewTree != null) {
> 2237: previewDiv.add(new HtmlTree(TagName.A).put(HtmlAttr.ID, 
> "preview")
> 2238:   .add(new
> RawHtml(utils.getPreviewTreeSummaryOrDetails(previewTree, false;

The `id` attribute needs to be unique within the page, so in addition to make 
the value not a valid java identifier (as
@jonathan-gibbons pointed out in a comment elsewhere) we need to support 
multiple preview ids per page. One way to do
this would be to add the element name to the id value, e.g. `preview-`.

-

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

Re: RFR: 8216497: javadoc should auto-link to platform classes [v3]

[Hannes Wallnöfer]

> This pull request is identical with the RFR previously sent for the Mercurial 
> repository:
> 
> https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html
> 
> I'm copy-pasting the comments from the original RFR below.
> 
> Most of the new code is added to the Extern class where it fits in quite 
> nicely and can use the existing supporting
> code for setting up external links.
> The default behaviour is to generate links to docs.oracle.com for released 
> versions and download.java.net for
> prereleases. Platform documentation URLs can be configured using the new 
> --link-platform-properties  option to
> provide a properties file with URLs pointing to to alternative locations. See 
> the CSR linked above for more details on
> the new options.  The feature can be disabled using the --no-platform-link 
> option, generating the same output as
> previously.   One problem I had to solve was how to handle the transition 
> from prerelease versions to final releases,
> since the location of the documentation changes in this transition. For 
> obvious reasons we don’t want to make that
> switch via code change at a time shortly before release.  The way it is done 
> is that we determine if the current
> javadoc instance is a prerelease version as indicated by the Version returned 
> by BaseConfiguration#getDocletVersion(),
> and then check whether the release/source version of the current javadoc 
> execution uses the same (latest) version. This
> means that that only the latest version will ever generate prerelease links 
> (e.g. running current javadoc 16 with
> source version 15 will generate links to the final release documentation) but 
> I think this is acceptable.  Another
> issue I spent some time getting right was tests. New releases require a new 
> element-list resource*), so tests have to
> pick up new releases. On the other hand, we don’t want hundreds of tests to 
> fail when a new release is added, ideally
> there should be one test  with a clear message. Because of this, when a 
> release is encountered for which no
> element-list is available a warning is generated instead of an error, and the 
> documentation is generated without
> platform links as if running with --no-platform-link option. This allows most 
> existing tests to pass and just the new
> test to fail with a relatively clear message of what is wrong.
> *) I also thought about generating the element-list for the current release 
> at build time. It’s quite involved, and we
>  still need to maintain element-lists for older versions, so I’m not sure 
> it’s worth it.
> 
> For existing tests that check output affected by the new option I added  the 
> --no-platform-link option to disable the
> feature. Otherwise we’d have to update those tests with each new release (or 
> else freeze the tests to use some static
> release or source version, which we don’t want either).  I updated the CSR to 
> the new code. It also needs to be
> reviewed: https://bugs.openjdk.java.net/browse/JDK-8251181
> 
> Thanks,
> Hannes

Hannes Wallnöfer has updated the pull request incrementally with one additional 
commit since the last revision:

  Rename --no-platform-link option plus minor cleanup

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/171/files
  - new: https://git.openjdk.java.net/jdk/pull/171/files/6d659ae3..6009dd70

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

  Stats: 73 lines in 38 files changed: 0 ins; 0 del; 73 mod
  Patch: https://git.openjdk.java.net/jdk/pull/171.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/171/head:pull/171

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

Re: RFR: 8216497: javadoc should auto-link to platform classes [v2]

[Hannes Wallnöfer]

On Tue, 22 Sep 2020 17:30:19 GMT, Jonathan Gibbons  wrote:

>> Hannes Wallnöfer has updated the pull request incrementally with three 
>> additional commits since the last revision:
>> 
>>  - Merge pull request #1 from lahodaj/JDK-8216497
>>
>>Automatically generate the elements-list data from the ct.sym for 
>> releases 11+, including the current release under
>>development
>>  - Generating current release list for javadoc; using hardcoded lists for 
>> versions < 11
>>  - Attempting to (mostly) generate the javadoc release manifests from ct.sym 
>> historical data.
>
> test/langtools/jdk/javadoc/doclet/testAnnotationTypes/TestAnnotationTypes.java
>  line 49:
> 
>> 47: javadoc("-d", "out-1",
>> 48: "-sourcepath", testSrc,
>> 49: "--no-platform-link",
> 
> I see lots of instances of `no-platform-link` in this and subsequent tests. 
> `JavadocTester` does have the concept of
> default options, although that may be more for the behavior after executing 
> javadoc and not for the options given to
> javadoc itself. Is it worth supporting default javadoc options, since that 
> the default can be disabled for specific
> tests?

I can't really find how `JavadocTester` uses or supports default options. My 
concern with this would be that it would
make JavadocTester less transparent and intuitive to use, as you'd have to be 
aware what the default options are.

-

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

Re: RFR: 8216497: javadoc should auto-link to platform classes [v2]

[Hannes Wallnöfer]

> This pull request is identical with the RFR previously sent for the Mercurial 
> repository:
> 
> https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html
> 
> I'm copy-pasting the comments from the original RFR below.
> 
> Most of the new code is added to the Extern class where it fits in quite 
> nicely and can use the existing supporting
> code for setting up external links.
> The default behaviour is to generate links to docs.oracle.com for released 
> versions and download.java.net for
> prereleases. Platform documentation URLs can be configured using the new 
> --link-platform-properties  option to
> provide a properties file with URLs pointing to to alternative locations. See 
> the CSR linked above for more details on
> the new options.  The feature can be disabled using the --no-platform-link 
> option, generating the same output as
> previously.   One problem I had to solve was how to handle the transition 
> from prerelease versions to final releases,
> since the location of the documentation changes in this transition. For 
> obvious reasons we don’t want to make that
> switch via code change at a time shortly before release.  The way it is done 
> is that we determine if the current
> javadoc instance is a prerelease version as indicated by the Version returned 
> by BaseConfiguration#getDocletVersion(),
> and then check whether the release/source version of the current javadoc 
> execution uses the same (latest) version. This
> means that that only the latest version will ever generate prerelease links 
> (e.g. running current javadoc 16 with
> source version 15 will generate links to the final release documentation) but 
> I think this is acceptable.  Another
> issue I spent some time getting right was tests. New releases require a new 
> element-list resource*), so tests have to
> pick up new releases. On the other hand, we don’t want hundreds of tests to 
> fail when a new release is added, ideally
> there should be one test  with a clear message. Because of this, when a 
> release is encountered for which no
> element-list is available a warning is generated instead of an error, and the 
> documentation is generated without
> platform links as if running with --no-platform-link option. This allows most 
> existing tests to pass and just the new
> test to fail with a relatively clear message of what is wrong.
> *) I also thought about generating the element-list for the current release 
> at build time. It’s quite involved, and we
>  still need to maintain element-lists for older versions, so I’m not sure 
> it’s worth it.
> 
> For existing tests that check output affected by the new option I added  the 
> --no-platform-link option to disable the
> feature. Otherwise we’d have to update those tests with each new release (or 
> else freeze the tests to use some static
> release or source version, which we don’t want either).  I updated the CSR to 
> the new code. It also needs to be
> reviewed: https://bugs.openjdk.java.net/browse/JDK-8251181
> 
> Thanks,
> Hannes

Hannes Wallnöfer has updated the pull request incrementally with three 
additional commits since the last revision:

 - Merge pull request #1 from lahodaj/JDK-8216497
   
   Automatically generate the elements-list data from the ct.sym for releases 
11+, including the current release under
   development
 - Generating current release list for javadoc; using hardcoded lists for 
versions < 11
 - Attempting to (mostly) generate the javadoc release manifests from ct.sym 
historical data.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/171/files
  - new: https://git.openjdk.java.net/jdk/pull/171/files/2aed84f8..6d659ae3

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

  Stats: 2007 lines in 9 files changed: 308 ins; 1698 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/171.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/171/head:pull/171

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

Re: RFR: 8216497: javadoc should auto-link to platform classes

[Hannes Wallnöfer]

On Tue, 15 Sep 2020 12:56:13 GMT, Erik Joelsson  wrote:

>> This pull request is identical with the RFR previously sent for the 
>> Mercurial repository:
>> 
>> https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html
>> 
>> I'm copy-pasting the comments from the original RFR below.
>> 
>> Most of the new code is added to the Extern class where it fits in quite 
>> nicely and can use the existing supporting
>> code for setting up external links.
>> The default behaviour is to generate links to docs.oracle.com for released 
>> versions and download.java.net for
>> prereleases. Platform documentation URLs can be configured using the new 
>> --link-platform-properties  option to
>> provide a properties file with URLs pointing to to alternative locations. 
>> See the CSR linked above for more details on
>> the new options.  The feature can be disabled using the --no-platform-link 
>> option, generating the same output as
>> previously.   One problem I had to solve was how to handle the transition 
>> from prerelease versions to final releases,
>> since the location of the documentation changes in this transition. For 
>> obvious reasons we don’t want to make that
>> switch via code change at a time shortly before release.  The way it is done 
>> is that we determine if the current
>> javadoc instance is a prerelease version as indicated by the Version 
>> returned by BaseConfiguration#getDocletVersion(),
>> and then check whether the release/source version of the current javadoc 
>> execution uses the same (latest) version. This
>> means that that only the latest version will ever generate prerelease links 
>> (e.g. running current javadoc 16 with
>> source version 15 will generate links to the final release documentation) 
>> but I think this is acceptable.  Another
>> issue I spent some time getting right was tests. New releases require a new 
>> element-list resource*), so tests have to
>> pick up new releases. On the other hand, we don’t want hundreds of tests to 
>> fail when a new release is added, ideally
>> there should be one test  with a clear message. Because of this, when a 
>> release is encountered for which no
>> element-list is available a warning is generated instead of an error, and 
>> the documentation is generated without
>> platform links as if running with --no-platform-link option. This allows 
>> most existing tests to pass and just the new
>> test to fail with a relatively clear message of what is wrong.
>> *) I also thought about generating the element-list for the current release 
>> at build time. It’s quite involved, and we
>>  still need to maintain element-lists for older versions, so I’m not sure 
>> it’s worth it.
>> 
>> For existing tests that check output affected by the new option I added  the 
>> --no-platform-link option to disable the
>> feature. Otherwise we’d have to update those tests with each new release (or 
>> else freeze the tests to use some static
>> release or source version, which we don’t want either).  I updated the CSR 
>> to the new code. It also needs to be
>> reviewed: https://bugs.openjdk.java.net/browse/JDK-8251181
>> 
>> Thanks,
>> Hannes
>
> Build changes look good.

Converted to draft as @lahodaj has offered to enhance the feature as described 
in the comments above.

-

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

Re: RFR: 8216497: javadoc should auto-link to platform classes

[Hannes Wallnöfer]

On Tue, 15 Sep 2020 11:30:09 GMT, Jan Lahoda  wrote:

>> This pull request is identical with the RFR previously sent for the 
>> Mercurial repository:
>> 
>> https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html
>> 
>> I'm copy-pasting the comments from the original RFR below.
>> 
>> Most of the new code is added to the Extern class where it fits in quite 
>> nicely and can use the existing supporting
>> code for setting up external links.
>> The default behaviour is to generate links to docs.oracle.com for released 
>> versions and download.java.net for
>> prereleases. Platform documentation URLs can be configured using the new 
>> --link-platform-properties  option to
>> provide a properties file with URLs pointing to to alternative locations. 
>> See the CSR linked above for more details on
>> the new options.  The feature can be disabled using the --no-platform-link 
>> option, generating the same output as
>> previously.   One problem I had to solve was how to handle the transition 
>> from prerelease versions to final releases,
>> since the location of the documentation changes in this transition. For 
>> obvious reasons we don’t want to make that
>> switch via code change at a time shortly before release.  The way it is done 
>> is that we determine if the current
>> javadoc instance is a prerelease version as indicated by the Version 
>> returned by BaseConfiguration#getDocletVersion(),
>> and then check whether the release/source version of the current javadoc 
>> execution uses the same (latest) version. This
>> means that that only the latest version will ever generate prerelease links 
>> (e.g. running current javadoc 16 with
>> source version 15 will generate links to the final release documentation) 
>> but I think this is acceptable.  Another
>> issue I spent some time getting right was tests. New releases require a new 
>> element-list resource*), so tests have to
>> pick up new releases. On the other hand, we don’t want hundreds of tests to 
>> fail when a new release is added, ideally
>> there should be one test  with a clear message. Because of this, when a 
>> release is encountered for which no
>> element-list is available a warning is generated instead of an error, and 
>> the documentation is generated without
>> platform links as if running with --no-platform-link option. This allows 
>> most existing tests to pass and just the new
>> test to fail with a relatively clear message of what is wrong.
>> *) I also thought about generating the element-list for the current release 
>> at build time. It’s quite involved, and we
>>  still need to maintain element-lists for older versions, so I’m not sure 
>> it’s worth it.
>> 
>> For existing tests that check output affected by the new option I added  the 
>> --no-platform-link option to disable the
>> feature. Otherwise we’d have to update those tests with each new release (or 
>> else freeze the tests to use some static
>> release or source version, which we don’t want either).  I updated the CSR 
>> to the new code. It also needs to be
>> reviewed: https://bugs.openjdk.java.net/browse/JDK-8251181
>> 
>> Thanks,
>> Hannes
>
> I think it would be awesome if we could generate (most of) the 
> {element,package}-list-VERSION.txt from the ct.sym
> historical data at build time. This would (hopefully) help with long-term 
> maintenance. I've started to sketch that
> here: 
> https://github.com/lahodaj/jdk/commit/36c1510587a4b050b148eda87ae7a7a89aff9690
> Some comments on the attempt:
> -in this PR, there is package-list-9.txt - should it be element-list-9.txt, 
> and should it contain module names (dtto
>  element-list-10.txt)?
> -we may (for historical reasons) want to keep the lists for 7, 8, 9 and 10 
> (as the historical data in ct.sym do not
>  exactly match what is in the package/element lists). It would be better to 
> generate everything, but having a fixed list
>  for some of the past versions would be better than having to create a new 
> list for each new release.
> -the patch does not yet generate the list for the current release, but that 
> should be doable.

Thanks for the suggestions and help, Jan!

> I think it would be awesome if we could generate (most of) the 
> {element,package}-list-VERSION.txt from the ct.sym
> historical data at build time. This would (hopefully) help with long-term 
> maintenance. I've started to sketch that
> here: 
> [lahodaj@36c1510](https://github.com/lahodaj/jdk/commit/36c1510587a4b050b148eda87ae7a7a89aff9690)

I agree files should be generated dynamically. I knew about the sym files but 
wasn't sure how to go about it. Thanks a
lot for stepping in and helping out, it's very much appreciated!

> Some comments on the attempt:
> -in this PR, there is package-list-9.txt - should it be element-list-9.txt, 
> and should it contain module names (dtto
>  element-list-10.txt)?

Javadoc in 9 still uses the old package-centric layout (package-list and no 
module names in URL paths). It 

RFR: 8216497: javadoc should auto-link to platform classes

[Hannes Wallnöfer]

This pull request is identical with the RFR previously sent for the Mercurial 
repository:

https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-August/001796.html

I'm copy-pasting the comments from the original RFR below.

Most of the new code is added to the Extern class where it fits in quite nicely 
and can use the existing supporting
code for setting up external links.

The default behaviour is to generate links to docs.oracle.com for released 
versions and download.java.net for
prereleases. Platform documentation URLs can be configured using the new 
--link-platform-properties  option to
provide a properties file with URLs pointing to to alternative locations. See 
the CSR linked above for more details on
the new options.

The feature can be disabled using the --no-platform-link option, generating the 
same output as previously.

One problem I had to solve was how to handle the transition from prerelease 
versions to final releases, since the
location of the documentation changes in this transition. For obvious reasons 
we don’t want to make that switch via
code change at a time shortly before release.

The way it is done is that we determine if the current javadoc instance is a 
prerelease version as indicated by the
Version returned by BaseConfiguration#getDocletVersion(), and then check 
whether the release/source version of the
current javadoc execution uses the same (latest) version. This means that that 
only the latest version will ever
generate prerelease links (e.g. running current javadoc 16 with source version 
15 will generate links to the final
release documentation) but I think this is acceptable.

Another issue I spent some time getting right was tests. New releases require a 
new element-list resource*), so tests
have to pick up new releases. On the other hand, we don’t want hundreds of 
tests to fail when a new release is added,
ideally there should be one test  with a clear message. Because of this, when a 
release is encountered for which no
element-list is available a warning is generated instead of an error, and the 
documentation is generated without
platform links as if running with --no-platform-link option. This allows most 
existing tests to pass and just the new
test to fail with a relatively clear message of what is wrong.

*) I also thought about generating the element-list for the current release at 
build time. It’s quite involved, and we
 still need to maintain element-lists for older versions, so I’m not sure it’s 
worth it.

For existing tests that check output affected by the new option I added  the 
--no-platform-link option to disable the
feature. Otherwise we’d have to update those tests with each new release (or 
else freeze the tests to use some static
release or source version, which we don’t want either).

I updated the CSR to the new code. It also needs to be reviewed:
https://bugs.openjdk.java.net/browse/JDK-8251181

Thanks,
Hannes

-

Commit messages:
 - 8216497: javadoc should auto-link to platform classes

Changes: https://git.openjdk.java.net/jdk/pull/171/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=171=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8216497
  Stats: 3230 lines in 53 files changed: 3220 ins; 4 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/171.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/171/head:pull/171

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

Re: RFR: JEP 359: Records (Preview) (full code)

[Hannes Wallnöfer]

Hi Vicente,

I’ve looked at the Javadoc changes again, everything looks good to me.

Hannes

> Am 28.11.2019 um 05:37 schrieb Vicente Romero :
> 
> Hi,
> 
> Please review the code for the records feature at [1]. This webrev includes 
> all: APIs, runtime, compiler, serialization, javadoc, and more! Must of the 
> code has been reviewed but there have been some changes since reviewers saw 
> it. Also this is the first time an integral webrev is sent out for review. 
> Last changes on top of my mind since last review iterations:
> 
> On the compiler implementation:
> - it has been adapted to the last version of the language spec [2], as a 
> reference the JVM spec is at [3]. This implied some changes in determining if 
> a user defined constructor is the canonical or not. Now if a constructor is 
> override-equivalent to a signature derived from the record components, then 
> it is considered the canonical constructor. And any canonical constructor 
> should satisfy a set of restrictions, see section 8.10.4 Record Constructor 
> Declarations of the specification.
> - It was also added a check to make sure that accessors are not generic.
> - And that the canonical constructor, if user defined, is not explicitly 
> invoking any other constructor.
> - The list of forbidden record component names has also been updated.
> - new error messages have been added
> 
> APIs:
> - there have been some API editing in java.lang.Record, 
> java.lang.runtime.ObjectMethods and java.lang.reflect.RecordComponent, 
> java.io.ObjectInputStream, javax.lang.model (some visitors were added)
> 
> On the JVM implementation:
> - some logging capabilities have been added to classFileParser.cpp to provide 
> the reason for which the Record attribute has been ignored
> 
> Reflection:
> - there are several new changes to the implementation of 
> java.lang.reflect.RecordComponent apart from the spec changes mentioned 
> before.
> 
> bug fixes in
> - compiler
> - serialization,
> - JVM, etc
> 
> As a reference the last iteration of the previous reviews can be found at [4] 
> under folders: compiler, hotspot_runtime, javadoc, reflection and 
> serialization,
> 
> TIA,
> Vicente
> 
> [1] http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.00/
> [2] 
> http://cr.openjdk.java.net/~gbierman/jep359/jep359-20191125/specs/records-jls.html
> [3] 
> http://cr.openjdk.java.net/~gbierman/jep359/jep359-20191125/specs/records-jvms.html
> [4] http://cr.openjdk.java.net/~vromero/records.review/
> 

Re: RFR 8214971 : Replace use of string.equals("") with isEmpty()

[Hannes Wallnöfer]

Hi Roger,

There are quite a few occurrences of the reverse calling pattern "".equals(str) 
in core libs.

This is a bit more tricky because it allows str to be null, but when used 
following a null/not-null check it could be replaced with isEmpty() as well.

I wonder if these should be included in the patch?

Hannes


> Am 06.12.2018 um 21:04 schrieb Roger Riggs :
> 
> Please review changing string.equals("") to string.isEmpty().
> isEmpty is preferred, performs better and is easier to optimize.
> 
> The change is a literal substitution in all files and only in these modules:
> 
>   java.base
>   java.logging
>   java.management
>   java.management.rmi
>   java.naming
>   java.net.http
>   java.prefs
>   java.rmi
>   java.scripting
>   java.sql
>   java.sql.rowset
>   java.xml
>   jdk.jartool
>   jdk.javadoc
>   jdk.jcmd
>   jdk.jconsole
>   jdk.management.agent
>   jdk.naming.dns
>   jdk.rmic
> 
> 
> Webrev:
> http://cr.openjdk.java.net/~rriggs/webrev-string-isempty-8214971-1/
> 
> Issue:
>   https://bugs.openjdk.java.net/browse/JDK-8214971
> 
> Thanks, Roger

Re: RFR: 8212726: Replace some use of drop- and foldArguments with filtering argument combinator in StringConcatFactory

[Hannes Wallnöfer]

This would definitely be a useful addition to the public MethodHandles API. 
Achieving this functionality with currently available means is extremely tricky 
and certainly does not produce readable or maintainable code.

Hannes

> Am 22.10.2018 um 14:07 schrieb Jim Laskey :
> 
> Thank you for doing this. The MethodHandle changes will simplify many a use 
> case.
> 
> Cheers,
> 
> — Jim
> 
> 
>> On Oct 22, 2018, at 6:58 AM, Claes Redestad  
>> wrote:
>> 
>> Hi,
>> 
>> StringConcatFactory uses a customized internal foldArguments
>> implementation which takes positional arguments to avoid intermediary
>> permutation steps, see JDK-8165492[1]. My intent has been to clean this
>> up for possible inclusion in the public API.
>> 
>> In preparation of that, I realized that we could probably profit from a
>> filtering combinator on top of the existing folding one. Said and done:
>> 
>> http://cr.openjdk.java.net/~redestad/8212726/jdk.00/
>> https://bugs.openjdk.java.net/browse/JDK-8212726
>> 
>> Using this new MethodHandles.filterArgumentsWithCombiner in place of
>> MHs.dropArguments + MHs.foldArgumentsWithCombiner gets rid of a
>> significant number of MethodHandles with retained performance
>> characteristics. In startup tests exercising indified String
>> concatenation the number of generated classes drops by ~25-30%, with a
>> measurable improvement in startup time as a result. Subjectively it also
>> makes the code in StringConcatFactory easier to read and follow.
>> 
>> While this surely strengthens the case to include these in the public
>> API, I'd like to move ahead with this as an internal optimization first
>> and propose MethodHandles.filter-/foldArgumentsWithCombiner as public
>> API points as a follow-up: apart from improving the javadoc we need to
>> work out specification for corner cases - especially illegal values -
>> and harden the implementation with more tests. I'm sure there might be
>> opinions about the naming of these methods, too... :-)
>> 
>> Thanks!
>> 
>> /Claes
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8165492
> 

Re: RFR: JDK-8203827: Upgrade JLine to 2.14.6

[Hannes Wallnöfer]

Hi Jan, 

Works as expected with jjs now.

Thanks,
Hannes

> Am 30.05.2018 um 17:06 schrieb Jan Lahoda :
> 
> Hi,
> 
> An updated webrev is here:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.01/complete/
> 
> A webrev showing changes from the previous revision is here:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.01/delta/
> 
> The changes are:
> -updated src/jdk.internal.le/share/legal/jline.md
> -the problem with automatically adding space after completion is resolved
> -a few tweaks to tests
> 
> Does this look good?
> 
> Thanks,
>Jan
> 
> On 29.5.2018 16:04, Jan Lahoda wrote:
>> Hi,
>> 
>> On 29.5.2018 14:51, Alan Bateman wrote:
>>> On 25/05/2018 21:20, Jan Lahoda wrote:
 Hi,
 
 I'd like to upgrade the JLine used by JShell and jjs from 2.12.1 to
 2.14.6.
 
 The complete webrev is here:
 http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/complete/
 
 To simplify reviewing, there is:
 -an antipatch that removes the JDK-specific changes and restores the
 vanilla 2.12.1 content:
 http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/undo-jdk-extras/
 -a patch that replaces the 2.12.1 content with 2.14.6:
 http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/upgrade-jline/
 -a patch that re-applies the JDK-specific changes (like including
 adjusting packages, and removal/commenting out of usage of features
 that would require undesirable dependencies, and any changes that had
 to be done to other modules):
 http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/adding-jdk-extras/
 
 JBS entry: https://bugs.openjdk.java.net/browse/JDK-8203827
 
 How does this look?
>>> The refresh looks okay and I see the package name has been adjusted for
>>> the new files.
>> 
>> Thanks.
>> 
>>> 
>>> Is there an update to src/jdk.internal.le/share/legal/jlink.md?
>>> Minimally I would assume the version at the top of the file needs to be
>>> updated.
>> 
>> Yes, it should, I forgot to do that. I'll send an update later.
>> 
>> Jan
>> 
>>> 
>>> -Alan

Re: RFR: JDK-8203827: Upgrade JLine to 2.14.6

[Hannes Wallnöfer]

Hi Jan,

Nashorn changes look good.

I noticed one slight change of behaviour in jjs. When I enter „java.m“ and hit 
the tab key, it autocompletes to „java.math “, adding a space character at the 
end. This is a bit inconvenient, and the old version of jline didn’t do that.

Hannes


> Am 25.05.2018 um 22:20 schrieb Jan Lahoda :
> 
> Hi,
> 
> I'd like to upgrade the JLine used by JShell and jjs from 2.12.1 to 2.14.6.
> 
> The complete webrev is here:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/complete/
> 
> To simplify reviewing, there is:
> -an antipatch that removes the JDK-specific changes and restores the vanilla 
> 2.12.1 content:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/undo-jdk-extras/
> -a patch that replaces the 2.12.1 content with 2.14.6:
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/upgrade-jline/
> -a patch that re-applies the JDK-specific changes (like including adjusting 
> packages, and removal/commenting out of usage of features that would require 
> undesirable dependencies, and any changes that had to be done to other 
> modules):
> http://cr.openjdk.java.net/~jlahoda/8203827/webrev.00/adding-jdk-extras/
> 
> JBS entry: https://bugs.openjdk.java.net/browse/JDK-8203827
> 
> How does this look?
> 
> Thanks,
>Jan

Re: JDK 9 RFR of JDK-8173864: Problem list src/jdk/nashorn/api/tree/test/ParseAPITest.java for some platforms

[Hannes Wallnöfer]

+1

Hannes

> Am 03.02.2017 um 09:57 schrieb Sundararajan Athijegannathan 
> :
> 
> +1
> 
> -Sundar
> 
> On 03/02/17, 10:24 AM, Amy Lu wrote:
>> src/jdk/nashorn/api/tree/test/ParseAPITest.java
>> 
>> This nashorn test keeps failing at linux-i586, windows-i586 and 
>> solaris-sparcv9 (JDK-8173863)
>> 
>> Please review this patch to put the test to problem list until issues 
>> addressed.
>> 
>> bug: https://bugs.openjdk.java.net/browse/JDK-8173864
>> webrev: http://cr.openjdk.java.net/~amlu/8173864/webrev.00/
>> 
>> Thanks,
>> Amy
>> 
>> 
>> --- old/test/ProblemList.txt2017-02-03 12:45:00.0 +0800
>> +++ new/test/ProblemList.txt2017-02-03 12:44:59.0 +0800
>> @@ -23,4 +23,4 @@
>> #
>> ### 
>> 
>> -# No nashorn tests are on the problem list.
>> +src/jdk/nashorn/api/tree/test/ParseAPITest.java8173863 
>> linux-i586,windows-i586,solaris-sparcv9
>> 

Re: RFR 8071588: The spec for javax.script.ScriptEngineFactory.getProgram() should specify NPEs thrown

[Hannes Wallnöfer]

+1

Hannes

> Am 19.10.2016 um 07:06 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8071588
> 
> jdk webrev: http://cr.openjdk.java.net/~sundar/8071588/jdk/webrev.00/
> 
> nashorn webrev:
> http://cr.openjdk.java.net/~sundar/8071588/nashorn/webrev.00/
> 
> Thanks
> 
> -Sundar
> 

Re: RFR 8071678: javax.script.ScriptContext setAttribute method should clarify behavior when GLOBAL_SCOPE is used and global scope object is null

[Hannes Wallnöfer]

+1

Hannes

> Am 18.10.2016 um 13:36 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review http://cr.openjdk.java.net/~sundar/8071678/webrev.00/ for
> https://bugs.openjdk.java.net/browse/JDK-8071678
> 
> Thanks,
> 
> -Sundar
> 

RFR: 8159031: jjs throws NoSuchFileException if ~/.jjs.history does not exist

[Hannes Wallnöfer]

Please review the following changes:

Bug: https://bugs.openjdk.java.net/browse/JDK-8159031
JDK webrev: http://cr.openjdk.java.net/~hannesw/8159031/jdk/webrev.00/
Nashorn webrev: http://cr.openjdk.java.net/~hannesw/8159031/nashorn/webrev.00/

These are two one-liners that add a file existence check in Nashorn and a null 
check in jline.

Thanks, 
Hannes