Re: RFR: 8255671: Bidi.reorderVisually has misleading exception messages
On Fri, 30 Oct 2020 22:23:30 GMT, Naoto Sato wrote: > Hi, > > Please review this simple message fix that follows JDK-8255242. > > Naoto Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/973
Re: RFR: 8254354: Add a withInvokeExactBehavior() VarHandle combinator [v11]
On Fri, 30 Oct 2020 16:17:06 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This patch adds an asExact() combinator to VarHandle, that will return a new
>> VarHandle that performs exact type checks, similar to
>> MethodHandle::invokeExact, to help developers catch inexact VarHandle usage,
>> which can lead to performance degradation.
>>
>> This is implemented using a boolean flag in VarForm. If the flag is set, the
>> exact type of the invocation is checked against the exact type in the
>> VarForm. If there is a mismatch, a WrongMethodTypeException is thrown.
>>
>> Other than that, there is also an asGeneric() combinator added that does the
>> inverse operation (thanks to Rémi for the suggestion). I've also added The
>> `@Hidden` annotation to the VarHandleGuards methods, as well as a
>> type-checking helper method called from the generic invocation lambda form,
>> so that the stack trace we get points at the location where the VarHandle is
>> being used.
>>
>> Thanks,
>> Jorn
>>
>> CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> behaviour -> behavior in javadoc as well
src/java.base/share/classes/java/lang/invoke/VarHandle.java line 1601:
> 1599: * @apiNote
> 1600: * Invoke-exact behavior guarantees that that upon invocation of an
> access mode method
> 1601: * the types an arity of the arguments must match the {@link
> #accessModeType(AccessMode) access mode type},
s/an arity/and arity/
-
PR: https://git.openjdk.java.net/jdk/pull/843
RFR: 8255671: Bidi.reorderVisually has misleading exception messages
Hi, Please review this simple message fix that follows JDK-8255242. Naoto - Commit messages: - 8255671: Bidi.reorderVisually has misleading exception messages Changes: https://git.openjdk.java.net/jdk/pull/973/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=973=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255671 Stats: 35 lines in 2 files changed: 32 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/973.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/973/head:pull/973 PR: https://git.openjdk.java.net/jdk/pull/973
Re: RFR: JDK-8254920: Application launched with jpackage produced .exe crashes JVM [v2]
On Fri, 30 Oct 2020 12:47:08 GMT, Andy Herrick wrote: >> JVM > > Andy Herrick has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8254920: Application launched with jpackage produced .exe crashes JVM Marked as reviewed by almatvee (Committer). - PR: https://git.openjdk.java.net/jdk/pull/940
Re: RFR: 8247781: Day periods support [v3]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed exception messages. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/6e1eade7..29c47afc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v2]
On Fri, 30 Oct 2020 10:33:28 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Addressed the following comments:
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515003422
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515005296
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515008862
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515030268
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515030880
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515032002
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515036803
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515037626
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515038069
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515039056
>
> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
> line 981:
>
>> 979:
>> 980: {"B", "Text(DayPeriod,SHORT)"},
>> 981: {"BB", "Text(DayPeriod,SHORT)"},
>
> "BB" and "BBB" are not defined to do anything in the CSR. Java should match
> CLDR/LDML rules here.
Fixed.
> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
> line 540:
>
>> 538: builder.appendDayPeriodText(TextStyle.FULL);
>> 539: DateTimeFormatter f = builder.toFormatter();
>> 540: assertEquals(f.toString(), "Text(DayPeriod,FULL)");
>
> This should be "DayPeriod(FULL)", because an end user might create a
> `TemporalField` with the name "DayPeriod" and the toString should be unique.
Fixed.
> src/java.base/share/classes/java/time/format/Parsed.java line 352:
>
>> 350: (fieldValues.containsKey(MINUTE_OF_HOUR) ?
>> fieldValues.get(MINUTE_OF_HOUR) : 0);
>> 351: if (!dayPeriod.includes(mod)) {
>> 352: throw new DateTimeException("Conflict found: " +
>> changeField + " conflict with day period");
>
> "conflict with day period" -> "conflicts with day period"
>
> Should also include `changeValue` and ideally the valid range
Fixed.
> src/java.base/share/classes/java/time/format/Parsed.java line 472:
>
>> 470: }
>> 471: if (dayPeriod != null) {
>> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) {
>
> Are we certain that the CLDR data does not contain day periods based on
> minutes as well as hours? This logic does not check against MINUTE_OF_HOUR
> for example. The logic also conflicts with the spec Javadoc that says
> MINUTE_OF_HOUR is validated.
MINUTE_OF_HOUR without HOUR_OF_DAY/HOUR_OF_AMPM may not make any sense, so it
is only validated in updateCheckDayPeriodConflict.
> src/java.base/share/classes/java/time/format/Parsed.java line 500:
>
>> 498: }
>> 499: }
>> 500: }
>
> Looking at the existing logic, the `AMPM_OF_DAY` field is completely ignored
> if there is no `HOUR_OF_AMPM` field. Thus, there is no validation to check
> `AMPM_OF_DAY` against `HOUR_OF_DAY`. This seems odd. (AMPM_OF_DAY = 0 and
> HOUR_OF_DAY=18 does not look like it throws an exception, when it probably
> should).
>
> On solution would be for `AMPM_OF_DAY` to be resolved to a day period at line
> 427, checking for conflicts with any parsed day period. (a small bug fix
> behavioural change)
There are cases where a period crosses midnight, e.g., 23:00-04:00 so it cannot
validate am/pm, so I decided to just override ampm with dayperiod without
validating.
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 1489:
>
>> 1487: Objects.requireNonNull(style, "style");
>> 1488: if (style != TextStyle.FULL && style != TextStyle.SHORT &&
>> style != TextStyle.NARROW) {
>> 1489: throw new IllegalArgumentException("Style must be either
>> full, short, or narrow");
>
> Nothing in the Javadoc describes this behaviour. It would make more sense to
> map FULL_STANDALONE to FULL and so on and not throw an exception.
Fixed.
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 1869:
>
>> 1867: } else if (cur == 'B') {
>> 1868: switch (count) {
>> 1869: case 1, 2, 3 ->
>> appendDayPeriodText(TextStyle.SHORT);
>
> I think this should be `case 1`. The 2 and 3 are not in the Javadoc, and I
> don't think they are in LDML. I note that patterns G and E do this though, so
> there is precedent.
Fixed.
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 5094:
>
>> 5092: @Override
>> 5093: public String toString() {
>> 5094: return "Text(DayPeriod," + textStyle + ")";
>
> Should be "DayPeriod(FULL)" to avoid possible `toString` clashes with the
> text printer/parser
Fixed.
>
Re: RFR: 8247781: Day periods support [v2]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressed the following comments: - https://github.com/openjdk/jdk/pull/938#discussion_r515003422 - https://github.com/openjdk/jdk/pull/938#discussion_r515005296 - https://github.com/openjdk/jdk/pull/938#discussion_r515008862 - https://github.com/openjdk/jdk/pull/938#discussion_r515030268 - https://github.com/openjdk/jdk/pull/938#discussion_r515030880 - https://github.com/openjdk/jdk/pull/938#discussion_r515032002 - https://github.com/openjdk/jdk/pull/938#discussion_r515036803 - https://github.com/openjdk/jdk/pull/938#discussion_r515037626 - https://github.com/openjdk/jdk/pull/938#discussion_r515038069 - https://github.com/openjdk/jdk/pull/938#discussion_r515039056 - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/7ac5fa03..6e1eade7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=00-01 Stats: 99 lines in 3 files changed: 48 ins; 2 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: JDK-8254920: Application launched with jpackage produced .exe crashes JVM [v2]
On Fri, 30 Oct 2020 12:47:08 GMT, Andy Herrick wrote: >> JVM > > Andy Herrick has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8254920: Application launched with jpackage produced .exe crashes JVM Marked as reviewed by asemenyuk (Committer). - PR: https://git.openjdk.java.net/jdk/pull/940
Integrated: 8214561: Use {@systemProperty} for definition of "java.util.prefs.PreferencesFactory" system property
On Fri, 30 Oct 2020 19:47:14 GMT, Brent Christian wrote:
> Using the {@systemProperty} tag for the "java.util.prefs.PreferencesFactory"
> system property will allow it to be found in the main javadoc search index.
>
> The updated doc build looks good.
This pull request has now been integrated.
Changeset: 0f486033
Author:Brent Christian
URL: https://git.openjdk.java.net/jdk/commit/0f486033
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
8214561: Use {@systemProperty} for definition of
"java.util.prefs.PreferencesFactory" system property
Reviewed-by: lancea
-
PR: https://git.openjdk.java.net/jdk/pull/965
Integrated: 8255690: in StringBuilder.subSequence
On Fri, 30 Oct 2020 19:30:14 GMT, Brent Christian wrote: > There are a couple of stray ""'s in the StringBuilder.subSequence() > documentation, which should be removed. The updated doc build looks good. This pull request has now been integrated. Changeset: 98a69ede Author:Brent Christian URL: https://git.openjdk.java.net/jdk/commit/98a69ede Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8255690: in StringBuilder.subSequence Reviewed-by: lancea - PR: https://git.openjdk.java.net/jdk/pull/963
Re: RFR: 8255690: in StringBuilder.subSequence
On Fri, 30 Oct 2020 19:38:45 GMT, Lance Andersen wrote: >> There are a couple of stray ""'s in the StringBuilder.subSequence() >> documentation, which should be removed. The updated doc build looks good. > > Looks fine Brent Thanks, Lance - PR: https://git.openjdk.java.net/jdk/pull/963
Re: RFR: 8214561: Use {@systemProperty} for definition of "java.util.prefs.PreferencesFactory" system property
On Fri, 30 Oct 2020 19:47:14 GMT, Brent Christian wrote:
> Using the {@systemProperty} tag for the "java.util.prefs.PreferencesFactory"
> system property will allow it to be found in the main javadoc search index.
>
> The updated doc build looks good.
Marked as reviewed by lancea (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/965
RFR: 8214561: Use {@systemProperty} for definition of "java.util.prefs.PreferencesFactory" system property
Using the {@systemProperty} tag for the "java.util.prefs.PreferencesFactory"
system property will allow it to be found in the main javadoc search index.
The updated doc build looks good.
-
Commit messages:
- 8214561: Use {@systemProperty} for definition of
java.util.prefs.PreferencesFactory system property
Changes: https://git.openjdk.java.net/jdk/pull/965/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=965=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8214561
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/965.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/965/head:pull/965
PR: https://git.openjdk.java.net/jdk/pull/965
Re: RFR: 8255690: in StringBuilder.subSequence
On Fri, 30 Oct 2020 19:30:14 GMT, Brent Christian wrote: > There are a couple of stray ""'s in the StringBuilder.subSequence() > documentation, which should be removed. The updated doc build looks good. Looks fine Brent - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/963
RFR: 8255690: in StringBuilder.subSequence
There are a couple of stray ""'s in the StringBuilder.subSequence() documentation, which should be removed. The updated doc build looks good. - Commit messages: - remove nbsp from AbstractStringBuilder.subSequence() Changes: https://git.openjdk.java.net/jdk/pull/963/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=963=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255690 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/963.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/963/head:pull/963 PR: https://git.openjdk.java.net/jdk/pull/963
Withdrawn: 8255128: linux x86 build failure with libJNIPoint.c
On Fri, 30 Oct 2020 12:47:59 GMT, Jorn Vernee wrote: > Guard libJNIPoint.c impl in `#ifdef _LP64` block > > This fixes a 32-bit build error This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/956
Re: RFR: 8254354: Add an asExact() VarHandle combinator [v11]
> Hi, > > This patch adds an asExact() combinator to VarHandle, that will return a new > VarHandle that performs exact type checks, similar to > MethodHandle::invokeExact, to help developers catch inexact VarHandle usage, > which can lead to performance degradation. > > This is implemented using a boolean flag in VarForm. If the flag is set, the > exact type of the invocation is checked against the exact type in the > VarForm. If there is a mismatch, a WrongMethodTypeException is thrown. > > Other than that, there is also an asGeneric() combinator added that does the > inverse operation (thanks to Rémi for the suggestion). I've also added The > `@Hidden` annotation to the VarHandleGuards methods, as well as a > type-checking helper method called from the generic invocation lambda form, > so that the stack trace we get points at the location where the VarHandle is > being used. > > Thanks, > Jorn > > CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: behaviour -> behavior in javadoc as well - Changes: - all: https://git.openjdk.java.net/jdk/pull/843/files - new: https://git.openjdk.java.net/jdk/pull/843/files/ff4c08d2..d49e3f30 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=843=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=843=09-10 Stats: 17 lines in 1 file changed: 0 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/843.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/843/head:pull/843 PR: https://git.openjdk.java.net/jdk/pull/843
Re: RFR: 8254354: Add an asExact() VarHandle combinator [v10]
> Hi, > > This patch adds an asExact() combinator to VarHandle, that will return a new > VarHandle that performs exact type checks, similar to > MethodHandle::invokeExact, to help developers catch inexact VarHandle usage, > which can lead to performance degradation. > > This is implemented using a boolean flag in VarForm. If the flag is set, the > exact type of the invocation is checked against the exact type in the > VarForm. If there is a mismatch, a WrongMethodTypeException is thrown. > > Other than that, there is also an asGeneric() combinator added that does the > inverse operation (thanks to Rémi for the suggestion). I've also added The > `@Hidden` annotation to the VarHandleGuards methods, as well as a > type-checking helper method called from the generic invocation lambda form, > so that the stack trace we get points at the location where the VarHandle is > being used. > > Thanks, > Jorn > > CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: - behaviour -> behaviour - Return same VarHandle instance when instance is already exact/non-exact - Changes: - all: https://git.openjdk.java.net/jdk/pull/843/files - new: https://git.openjdk.java.net/jdk/pull/843/files/3c707bc7..ff4c08d2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=843=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=843=08-09 Stats: 172 lines in 10 files changed: 32 ins; 0 del; 140 mod Patch: https://git.openjdk.java.net/jdk/pull/843.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/843/head:pull/843 PR: https://git.openjdk.java.net/jdk/pull/843
Re: RFR: 8254354: Add an asExact() VarHandle combinator [v9]
On Thu, 29 Oct 2020 18:14:05 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This patch adds an asExact() combinator to VarHandle, that will return a new
>> VarHandle that performs exact type checks, similar to
>> MethodHandle::invokeExact, to help developers catch inexact VarHandle usage,
>> which can lead to performance degradation.
>>
>> This is implemented using a boolean flag in VarForm. If the flag is set, the
>> exact type of the invocation is checked against the exact type in the
>> VarForm. If there is a mismatch, a WrongMethodTypeException is thrown.
>>
>> Other than that, there is also an asGeneric() combinator added that does the
>> inverse operation (thanks to Rémi for the suggestion). I've also added The
>> `@Hidden` annotation to the VarHandleGuards methods, as well as a
>> type-checking helper method called from the generic invocation lambda form,
>> so that the stack trace we get points at the location where the VarHandle is
>> being used.
>>
>> Thanks,
>> Jorn
>>
>> CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375
>
> Jorn Vernee 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 12 additional
> commits since the last revision:
>
> - Update Javadoc, and rename asExact and asGeneric to
> withInvokeExactBehaviour and withInvokeBehaviour
> - Merge branch 'master' into Exact_VarHandle
> - Use AccessType ordinal in guard checks instead of the AccessMode ordinal
> - Update accessModeType to use the AccessType ordinal directly.
> - Add benchmarks
> - - Update javadoc
>- Make isExact() public
> - Fixes failing tests, and enable verifier on Exact test
> - Fix whitespace
> - Comment out VarHandleGuards generator code
> - Makes exactness a property of a VarHandle, not a VarForm, since the latter
> are shared. Use handle.accessModeType to get the exact type of the VarHandle.
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/08226775...3c707bc7
Looks good. There is just one difference between the spec and implementation.
The spec states:
> If this VarHandle already has invoke{-exact} behaviour this VarHandle is
> returned.
I prefer this behaviour, but feel free to update the spec if you like e.g. If
... already has XXX then a new VH with exactly the same behaviour as this VH is
returned.
I just realized i used "behaviour", the english spelling. We should use the US
spelling, "behavior", which is more commonly used throughout the JDK
documentation.
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/843
Re: RFR: 8255398: Add a dropReturn MethodHandle combinator [v3]
On Tue, 27 Oct 2020 10:40:40 GMT, Jorn Vernee wrote:
>>> …`dropReturn` seemed like a good choice since we already have
>>> `dropArguments`. WRT changing to `MethodHandle::changeReturnType`...
>>
>> That's a very reasonable point. People might look for `dropRT` after they
>> find `dropAs`. And `MHs` is designed as a large-ish set of utility methods.
>>
>> I agree that adding `MH::changeRT` is a slippery slope; it tends to lift
>> more of the MT API onto MH, and it starts to put new stuff into the smaller
>> MH API; that was my bad.
>>
>> But (in the spirit of brainstorming, and agreeing with your present
>> proposal), I'll suggest a separate idea to think about. Use a HOFP API to
>> give easy access to the entire `MT` API all in one go, by reifying the
>> `MH.type` property as a temporary (lambda argument):
>>
>> MethodHandle mapType(MethodHandle mh, UnaryOperator fn) {
>>return mh.asType(fn.apply(mh.type()));
>> }
>>
>> This also suggests a possible argument transform utility, a sort of
>> mini-version of Charlie Nutter's MH builder API:
>>
>> MethodHandle mapArguments(MethodHandle mh,
>> UnaryOperator>> fn) {
>> var args = mh.type().argumentList().stream().map(t -> makeArg(t)).asList();
>> args = fn.apply(args);
>> … do stuff to re-weave mh with the resulting argument transforms …
>> }
>> public sealed interface ArgumentValue {
>> Class type();
>>ArgumentValue asType(Class newType);
>> … other stuff for pre-applying MHs …
>> }
>
> It's an interesting idea. It turns something like:
>
> target = target.asType(target.type().changeReturnType(void.class));
>
> Into
>
> target = target.asType(mt -> mt.changeReturnType(void.class));
>
> I.e. it removes the need to reference the target handle/type twice, which
> prevents them from potentially going out of sync (which seems more likely
> when you have multiple MethodTypes and MethodHandles flying around). Also, it
> feels unnecessary that, I have to re-specify `target`'s type as a base, when
> I'm asking `target` to change it's type. This also solves that.
CSR is now provisional. Please review:
https://bugs.openjdk.java.net/browse/JDK-8255398
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/866
RFR: 8255128: linux x86 build failure with libJNIPoint.c
Guard libJNIPoint.c impl in `#ifdef _LP64` block This fixes a 32-bit build error - Commit messages: - Guard libJNIPoint.c impl in #ifdef _LP64 block Changes: https://git.openjdk.java.net/jdk/pull/956/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=956=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255128 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/956.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/956/head:pull/956 PR: https://git.openjdk.java.net/jdk/pull/956
Re: RFR: JDK-8254920: Application launched with jpackage produced .exe crashes JVM [v2]
> JVM Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8254920: Application launched with jpackage produced .exe crashes JVM - Changes: - all: https://git.openjdk.java.net/jdk/pull/940/files - new: https://git.openjdk.java.net/jdk/pull/940/files/196a9def..3d83b99b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=940=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=940=00-01 Stats: 16 lines in 2 files changed: 3 ins; 0 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/940.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/940/head:pull/940 PR: https://git.openjdk.java.net/jdk/pull/940
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v14]
> This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and associated pull request [3]). > > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate JNI glue code. In order to do > this, native calls are modeled through the MethodHandle API. I suggest > reading the writeup [4] I put together few weeks ago, which illustrates what > the foreign linker support is, and how it should be used by clients. > > Disclaimer: the pull request mechanism isn't great at managing *dependent* > reviews. For this reasons, I'm attaching a webrev which contains only the > differences between this PR and the memory access PR. I will be periodically > uploading new webrevs, as new iterations come out, to try and make the life > of reviewers as simple as possible. > > A big thank to Jorn Vernee and Vladimir Ivanov - they are the main architects > of all the hotspot changes you see here, and without their help, the foreign > linker support wouldn't be what it is today. As usual, a big thank to Paul > Sandoz, who provided many insights (often by trying the bits first hand). > > Thanks > Maurizio > > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library by name, or absolute path, > and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory layouts for the function > arguments/return type. A function descriptor is used to describe the > signature of a native function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes a native symbol (as > obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` and > returns a `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, and a > `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a > code stub allocated by the VM which acts as a trampoline from native code to > the user-provided method handle. This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures (e.g. `C_LONG` and > friends); these layouts contain additional ABI classfication information (in > the form of layout attributes) which is used by the runtime to *infer* how > Java arguments should be shuffled for the native call to take place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than allocating separate memory segments > using separate *try-with-resource* constructs, a `NativeScope` allows clients > to use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a performance boost, since not all > allocation requests will be turned into `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For instance, the description of the > native signature might be wrong (e.g. have too many arguments) - and the > runtime has, in the general case, no way to detect such mismatches. For these > reasons, obtaining a `CLinker` instance is a *restricted* operation, which > can be enabled by specifying the usual JDK property > `-Dforeign.restricted=permit` (as it's the case for other restricted method > in the foreign memory API). > > ### Implementation changes > > The Java changes associated with `LibraryLookup` are relative > straightforward; the only interesting thing to note here is that library > loading does _not_ depend on class loaders, so `LibraryLookup` is not subject > to the same restrictions which apply to JNI library loading (e.g. same > library
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v13]
> This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and associated pull request [3]). > > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate JNI glue code. In order to do > this, native calls are modeled through the MethodHandle API. I suggest > reading the writeup [4] I put together few weeks ago, which illustrates what > the foreign linker support is, and how it should be used by clients. > > Disclaimer: the pull request mechanism isn't great at managing *dependent* > reviews. For this reasons, I'm attaching a webrev which contains only the > differences between this PR and the memory access PR. I will be periodically > uploading new webrevs, as new iterations come out, to try and make the life > of reviewers as simple as possible. > > A big thank to Jorn Vernee and Vladimir Ivanov - they are the main architects > of all the hotspot changes you see here, and without their help, the foreign > linker support wouldn't be what it is today. As usual, a big thank to Paul > Sandoz, who provided many insights (often by trying the bits first hand). > > Thanks > Maurizio > > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library by name, or absolute path, > and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory layouts for the function > arguments/return type. A function descriptor is used to describe the > signature of a native function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes a native symbol (as > obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` and > returns a `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, and a > `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a > code stub allocated by the VM which acts as a trampoline from native code to > the user-provided method handle. This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures (e.g. `C_LONG` and > friends); these layouts contain additional ABI classfication information (in > the form of layout attributes) which is used by the runtime to *infer* how > Java arguments should be shuffled for the native call to take place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than allocating separate memory segments > using separate *try-with-resource* constructs, a `NativeScope` allows clients > to use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a performance boost, since not all > allocation requests will be turned into `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For instance, the description of the > native signature might be wrong (e.g. have too many arguments) - and the > runtime has, in the general case, no way to detect such mismatches. For these > reasons, obtaining a `CLinker` instance is a *restricted* operation, which > can be enabled by specifying the usual JDK property > `-Dforeign.restricted=permit` (as it's the case for other restricted method > in the foreign memory API). > > ### Implementation changes > > The Java changes associated with `LibraryLookup` are relative > straightforward; the only interesting thing to note here is that library > loading does _not_ depend on class loaders, so `LibraryLookup` is not subject > to the same restrictions which apply to JNI library loading (e.g. same > library
Re: RFR: 8247781: Day periods support
On Thu, 29 Oct 2020 15:59:51 GMT, Naoto Sato wrote:
> Hi,
>
> Please review the changes for the subject issue. This is to enhance the
> java.time package to support day periods, such as "in the morning", defined
> in CLDR. It will add a new pattern character 'B' and its supporting builder
> method. The motivation and its spec are in this CSR:
>
> https://bugs.openjdk.java.net/browse/JDK-8254629
>
> Naoto
Changes requested by scolebourne (Author).
test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line
981:
> 979:
> 980: {"B", "Text(DayPeriod,SHORT)"},
> 981: {"BB", "Text(DayPeriod,SHORT)"},
"BB" and "BBB" are not defined to do anything in the CSR. Java should match
CLDR/LDML rules here.
test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line
540:
> 538: builder.appendDayPeriodText(TextStyle.FULL);
> 539: DateTimeFormatter f = builder.toFormatter();
> 540: assertEquals(f.toString(), "Text(DayPeriod,FULL)");
This should be "DayPeriod(FULL)", because an end user might create a
`TemporalField` with the name "DayPeriod" and the toString should be unique.
src/java.base/share/classes/java/time/format/Parsed.java line 352:
> 350: (fieldValues.containsKey(MINUTE_OF_HOUR) ?
> fieldValues.get(MINUTE_OF_HOUR) : 0);
> 351: if (!dayPeriod.includes(mod)) {
> 352: throw new DateTimeException("Conflict found: " +
> changeField + " conflict with day period");
"conflict with day period" -> "conflicts with day period"
Should also include `changeValue` and ideally the valid range
src/java.base/share/classes/java/time/format/Parsed.java line 472:
> 470: }
> 471: if (dayPeriod != null) {
> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) {
Are we certain that the CLDR data does not contain day periods based on minutes
as well as hours? This logic does not check against MINUTE_OF_HOUR for example.
The logic also conflicts with the spec Javadoc that says MINUTE_OF_HOUR is
validated.
src/java.base/share/classes/java/time/format/Parsed.java line 497:
> 495: AMPM_OF_DAY.checkValidValue(ap);
> 496: }
> 497: updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint /
> 720);
No need to put `AMPM_OF_DAY` back in here because you've already resolved it to
`HOUR_OF_DAY` and `MINUTE_OF_HOUR`. There probably does need to be validation
to check that the day period agrees with the AM/PM value.
src/java.base/share/classes/java/time/format/Parsed.java line 500:
> 498: }
> 499: }
> 500: }
Looking at the existing logic, the `AMPM_OF_DAY` field is completely ignored if
there is no `HOUR_OF_AMPM` field. Thus, there is no validation to check
`AMPM_OF_DAY` against `HOUR_OF_DAY`. This seems odd. (AMPM_OF_DAY = 0 and
HOUR_OF_DAY=18 does not look like it throws an exception, when it probably
should).
On solution would be for `AMPM_OF_DAY` to be resolved to a day period at line
427, checking for conflicts with any parsed day period. (a small bug fix
behavioural change)
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
1489:
> 1487: Objects.requireNonNull(style, "style");
> 1488: if (style != TextStyle.FULL && style != TextStyle.SHORT &&
> style != TextStyle.NARROW) {
> 1489: throw new IllegalArgumentException("Style must be either
> full, short, or narrow");
Nothing in the Javadoc describes this behaviour. It would make more sense to
map FULL_STANDALONE to FULL and so on and not throw an exception.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
1869:
> 1867: } else if (cur == 'B') {
> 1868: switch (count) {
> 1869: case 1, 2, 3 ->
> appendDayPeriodText(TextStyle.SHORT);
I think this should be `case 1`. The 2 and 3 are not in the Javadoc, and I
don't think they are in LDML. I note that patterns G and E do this though, so
there is precedent.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5094:
> 5092: @Override
> 5093: public String toString() {
> 5094: return "Text(DayPeriod," + textStyle + ")";
Should be "DayPeriod(FULL)" to avoid possible `toString` clashes with the text
printer/parser
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5153:
> 5151: * minute-of-day of "at" or "from" attribute
> 5152: */
> 5153: private final long from;
Could these three instance variables be `int` ?
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5252:
> 5250:
> .getLocaleResources(CalendarDataUtility.findRegionOverride(l));
> 5251: String dayPeriodRules = lr.getRules()[1];
> 5252: final
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v19]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can largely ignore this interface, which comes in > really handy when
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v5]
> 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 44 commits: - Updating tests after records are a final feature. - Fixing tests. - Finalizing removal of record preview hooks. - Merging master into JDK-8250768 - Reflecting review comments. - Merge branch 'master' into JDK-8250768 - Removing unnecessary cast. - Using a more correct way to get URLs. - Reflecting review comments. - Merge branch 'JDK-8250768-dev' of https://github.com/lahodaj/jdk into JDK-8250768 - ... and 34 more: https://git.openjdk.java.net/jdk/compare/ea26ff11...d76eb293 - Changes: https://git.openjdk.java.net/jdk/pull/703/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=703=04 Stats: 3012 lines in 142 files changed: 2521 ins; 260 del; 231 mod Patch: https://git.openjdk.java.net/jdk/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703 PR: https://git.openjdk.java.net/jdk/pull/703
