Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Fri, 13 May 2022 04:41:03 GMT, ExE Boss wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updated copyrights >> Fixed cast style to add a space after cast, (where consistent with file >> style) >> Improved code per review comments in PollSelectors. > > src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128: > >> 126: // timed poll interrupted so need to adjust timeout >> 127: long adjust = System.nanoTime() - startTime; >> 128: to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust); > > This will now always assign a negative number to `to`. > > > > `=-` is not a compound assignment, it’s negation followed by a normal > assignment. Well spotted, I don't think that change was intentionally. - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Wed, 11 May 2022 16:30:41 GMT, Roger Riggs wrote: >> PR#8599 8244681: proposes to add compiler warnings for possible lossy >> conversions >> From the CSR: >> >> "If the type of the right-hand operand of a compound assignment is not >> assignment compatible with the type of the variable, a cast is implied and >> possible lossy conversion may silently occur. While similar situations are >> resolved as compilation errors for primitive assignments, there are no >> similar rules defined for compound assignments." >> >> This PR anticipates the warnings and adds explicit casts to replace the >> implicit casts. >> In most cases, the cast matches the type of the right-hand side to type of >> the compound assignment. >> Since these casts are truncating the value, there might be a different >> refactoring that avoids the cast >> but a direct replacement was chosen here. >> >> (Advise and suggestions will inform similar updates to other OpenJDK >> modules). > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Updated copyrights > Fixed cast style to add a space after cast, (where consistent with file > style) > Improved code per review comments in PollSelectors. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]
On Tue, 10 May 2022 23:01:33 GMT, Roger Riggs wrote:
>> PR#8599 8244681: proposes to add compiler warnings for possible lossy
>> conversions
>> From the CSR:
>>
>> "If the type of the right-hand operand of a compound assignment is not
>> assignment compatible with the type of the variable, a cast is implied and
>> possible lossy conversion may silently occur. While similar situations are
>> resolved as compilation errors for primitive assignments, there are no
>> similar rules defined for compound assignments."
>>
>> This PR anticipates the warnings and adds explicit casts to replace the
>> implicit casts.
>> In most cases, the cast matches the type of the right-hand side to type of
>> the compound assignment.
>> Since these casts are truncating the value, there might be a different
>> refactoring that avoids the cast
>> but a direct replacement was chosen here.
>>
>> (Advise and suggestions will inform similar updates to other OpenJDK
>> modules).
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove stray file
src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128:
> 126: // timed poll interrupted so need to adjust timeout
> 127: long adjust = System.nanoTime() - startTime;
> 128: to -= (int)TimeUnit.MILLISECONDS.convert(adjust,
> TimeUnit.NANOSECONDS);
Can we change this `to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);` while
we here?
src/java.base/share/classes/jdk/internal/org/objectweb/asm/Frame.java line 615:
> 613: if (outputStackTop >= elements) {
> 614: outputStackTop -= (short)elements;
> 615: } else {
I think we usually try to avoid changing the ASM code if possible but maybe you
have to do this because the compiler option is used for compiling java.base?
src/java.base/unix/classes/sun/nio/ch/PollSelectorImpl.java line 123:
> 121: // timed poll interrupted so need to adjust timeout
> 122: long adjust = System.nanoTime() - startTime;
> 123: to -= (int)TimeUnit.MILLISECONDS.convert(adjust,
> TimeUnit.NANOSECONDS);
This one can also be changed to:
`to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);`
-
PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8285370: Fix typo in jdk.charsets
On Thu, 21 Apr 2022 11:34:14 GMT, Magnus Ihse Bursie wrote:
> `codespell` could just find a single typo in the files covered by the i18n
> alias. Just fixing the typo did not make the comment more readable, so I
> rewrote it slightly.
Marked as reviewed by alanb (Reviewer).
src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM942C.java.template line 69:
> 67:
> 68: static {
> 69: // the mappings that need updating are
It probably meant to say "needing" but what you have is okay.
-
PR: https://git.openjdk.java.net/jdk/pull/8335
Re: RFR: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. I skimmed through the changes to src/java.base and they look okay except for the changes to 3rd party code that I think should be dropped from this patch. src/java.base/share/native/libzip/zlib/inftrees.h line 65: > 63:1444, which is the sum of 852 for literal/length codes and 592 for > distance > 64:codes. These values were found by exhaustive searches using the > program > 65:examples/enough.c found in the zlib distribution. The arguments to > that This is 3rd party code so should be dropped from the patch as we want as few changes to this code has possible. src/java.base/windows/native/libnio/ch/wepoll.c line 894: > 892: * error code when the once-callback returns FALSE. We return -1 > here to > 893: * indicate that global initialization failed; the failing init > function is > 894: * responsible for setting `errno` and calling `SetLastError()`. */ This is also 3rd party code so should be dropped from this patch. - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8257733: Move module-specific data from make to respective module [v9]
On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typos Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v9]
On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typos Thanks for dropping the charset and locale data from the proposal. The updated proposal (b1d1e4d8) looks okay but I can't tell if you are planning to integrate this or wait until there is discussion on the locations proposed in the Informational JEP that you've just submitted. Maybe you plan to just integrate and then adjust/move again if needed? I suspect that JEP will need to includes a "specs" directory. It's okay to jdwp.spec into the java.se "data" directory for now I think "specs" would be a bette place for it. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On 16/03/2022 08:44, Magnus Ihse Bursie wrote: : If you have such strong opinions on the data files shared between java.base and jdk.charsets/jdk.localedata, I propose we leave them in make/data for the time being, clean up the associated mess, and then work out where they actually should be. Does that sound okay to you? The concern, as before, is that it puts data files into src/java.base that are used by the build to generate classes/resources for the service provider modules. We also have the complication that the charsets to include in java.base varies by platform so the module/package for each charset is decided at build time. It's always been low-priority to re-visit that and not clear if we could even get to an agreement easily because there are IBM platforms that want EBCDIC and other double byte charsets whereas other platforms don't want these in java.base. So yes, if you can drop the move of the charset data and CLDR data from the patch then it will make it easier to discuss. You asked about the JDWP spec. This is the protocol spec that is used to generate the spec in HTML, and source files for the debugger front-end and backend (jdk.jdi and jdk.jdwp.agent modules). The "specs" directory might be right. There is another source file (jdwp-spec.md) that isn't in the src tree right now and they probably go together. -Alan
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
Magnus,
This proposal does not address the previous concerns. As before, you are
proposing to put the data files used to generate the classes for
jdk.localedata and jdk.charsets into src/java.base and I don't think we
should do that. I really think this PR needs to be withdraw n or closed
until there is at least some agreement on placement. I know you want to
get the files moved out of the make tree but there are many issues to
work through before that can happen.
-Alan
On 15/03/2022 23:59, Magnus Ihse Bursie wrote:
On Tue, 15 Mar 2022 23:50:20 GMT, Magnus Ihse Bursie wrote:
A lot (but not all) of the data in make/data is tied to a specific module. For
instance, the publicsuffixlist is used by java.base, and fontconfig by
java.desktop. (A few directories, like mainmanifest, is *actually* used by make
for the whole build.)
These data files should move to the module they belong to. The are, after all, "source code" for that module
that is "compiler" into resulting deliverables, for that module. (But the "source code" language is
not Java or C, but typically a highly domain specific language or data format, and the "compilation" is,
often, a specialized transformation.)
This misplacement of the data directory is most visible at code review time.
When such data is changed, most of the time build-dev (or the new build label)
is involved, even though this has nothing to do with the build. While this is
annoying, a worse problem is if the actual team that needs to review the patch
(i.e., the team owning the module) is missed in the review.
### Modules reviewed
- [x] java.base
- [x] java.desktop
- [x] jdk.compiler
- [x] java.se
Magnus Ihse Bursie has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 12 commits:
- Merge branch 'master' into shuffle-data-reborn
- Fix merge
- Merge tag 'jdk-19+13' into shuffle-data-reborn
Added tag jdk-19+13 for changeset 5df2a057
- Move characterdata templates to share/classes/java/lang.
- Update comment refering to "make" dir
- Move new symbols to jdk.compiler
- Merge branch 'master' into shuffle-data
- Move macosxicons from share to macosx
- Move to share/data, and move jdwp.spec to java.se
- Update references in test
- ... and 2 more: https://git.openjdk.java.net/jdk/compare/83d77186...598f740f
I have carefully reviewed all PR comments, and the changes I made in response
to them. I believe I have resolved all requests from reviewers. What remained
to do was to create an informational JEP about the new source structure, and
file some follow-up issues.
I have now created and submitted a new informational JEP ("JDK Source
Structure"), available at https://bugs.openjdk.java.net/browse/JDK-8283227. When
creating this JEP, it felt increasingly silly to just copy and extend the part about
src/$MODULE from JEP 201, so I extended it to cover a relevant overview of the entire JDK
source base structure. I actually think this JEP has a good merit on its own,
notwithstanding it being a reviewer requirement for this PR.
I have also filed follow up issues for the non-standard jdk.hotspot.agent `doc`
and `test` directories (https://bugs.openjdk.java.net/browse/JDK-8283197 and
https://bugs.openjdk.java.net/browse/JDK-8283198, respectively).
I have filed a follow up issue for continued efforts to clean up
charsetmapping, https://bugs.openjdk.java.net/browse/JDK-8283228.
There were two open questions:
* should jdwp.spec belong to specs directory instead of data
* should bin/idea.sh be changed to exclude data
but they sounded so exploratory that I decided not to open JBS issues for them.
@wangweij @naotoj @prrace @erikj79 @jonathan-gibbons You have all approved
this PR at an older revision. Can you please reconfirm that your approval
stands for the latest revision? (Sorry for the mass ping)
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8282887: Potential memory leak in sun.util.locale.provider.HostLocaleProviderAdapterImpl.getNumberPattern() on Windows
On Thu, 10 Mar 2022 18:40:13 GMT, Zhengyu Gu wrote: > Please review this small patch that fixes early `CHECK_NULL_RETURN` results > not releasing native `jchar` array returned by `GetStringChars()`. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/
Re: RFR: 8280474: Garbage value passed to getLocaleInfoWrapper in HostLocaleProviderAdapter_md
On Fri, 21 Jan 2022 19:28:21 GMT, Daniel Jeliński wrote: > Reported by clang-tidy. Verified manually by running > > Calendar c = Calendar.getInstance(); > System.out.println (c.getDisplayNames(Calendar.MONTH, > Calendar.SHORT_STANDALONE, Locale.getDefault())); > > with `-Djava.locale.providers=HOST` > > Without the fix the WINAPI functions fail, and [this > block](https://github.com/openjdk/jdk/blame/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/windows/native/libjava/HostLocaleProviderAdapter_md.c#L857) > is never entered. With the fix this block is entered and sensible values are > stored in the returned array. > > No test because the observable effect with and without the fix was the same; > apparently there's a fallback to another provider that works. Good find. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7184
Re: RFR: 8277398: javac does not accept encoding name COMPAT [v2]
On Wed, 24 Nov 2021 04:08:43 GMT, Ichiroh Takiguchi wrote: >> ncoding name COMPAT was defined for operating system encoding by JEP-400. >> https://openjdk.java.net/jeps/400 >> But java does not accept "-encoding COMPAT". > > Ichiroh Takiguchi has updated the pull request incrementally with one > additional commit since the last revision: > > 8277398: javac does not accept encoding name COMPAT I see this PR has been re-purposed to add "COMPAT" as a charset that can be specified to Charset.forName. I don't think we should do that. "COMPAT" is a special value for the file.encoding property, it's not meant to be in the charset tables as proposed here. The system property "native.encoding" was added in Java 17 as a standard way to obtain the encoding, you can pass its value to Charset.forName. I think we need a clear summary as to what the issue is, is -J-Dfile.encoding=COMPAT working or not? - PR: https://git.openjdk.java.net/jdk/pull/6475
Re: RFR: 8277398: javac does not accept encoding name COMPAT
On Fri, 19 Nov 2021 07:00:44 GMT, Ichiroh Takiguchi
wrote:
> ncoding name COMPAT was defined for operating system encoding by JEP-400.
> https://openjdk.java.net/jeps/400
> But java does not accept "-encoding COMPAT".
src/jdk.compiler/share/classes/com/sun/tools/javac/file/BaseFileManager.java
line 283:
> 281: if (enc != null) {
> 282: encodingName = enc;
> 283: }
If we updating javac and javadoc --encoding to support "COMPAT" then we should
list this in JEP 400.
Does javadoc use doPriv already, I don't know how common it would be to run
javadoc with a SM set. If the doPriv stays then you can avoid the cast by
changing it to:
PrivilegedAction pa = () -> System.getProperty("native.encoding");
return AccessController.doPrivileged(pa);
-
PR: https://git.openjdk.java.net/jdk/pull/6475
Re: Supporting charset GB18030-2005
On 16/11/2021 19:02, Pushkar N Kulkarni wrote: Hi Alan, Thanks. I appreciate your response. Yes, I think GB13080 must continue to be GB13080-2000 for now. I was initially hoping to add a new character set with the name GB13080-2005. But I guess your suggestion of internally mapping one of the two versions (2000 or 2005) to "GB13080", based on the value of a new System property, version 2000 being the default, could be a better approach. We could start out by adding GB18030-2005, as you suggest. A potential next step would be to rename GB13080 to GB13080-2000, with "GB13080" as an alias. As it stands, the charset name is "GB13080" with "GB13080-2000" as an alias so it should be compatible with code that use Charset.forName. It's possible this change may be noticed by code that does lookups in other ways or expects getName to be match the name specified to forName so that would be a feature release only change. If there is a strong need then it should be feasible to have a system property to change GB13080 but maybe it's not needed in the short/medium term when some operating systems are still using -2000. -Alan
Re: Supporting charset GB18030-2005
On 15/11/2021 17:53, Pushkar N Kulkarni wrote: Hi there, OpenJDK currently supports version 2000 of the GB18030 (https://en.wikipedia.org/wiki/GB_18030) character set viz. GB18030-2000. The character mappings corresponding to Unicode codepoints '\u1E3F' and '\uE7C7' were swapped in a new version of the character set named GB18030-2005. I learn that this corrected a mistake in version 2000. OpenJDK does not support version 2005 as yet. Can someone please help me with reasons for the same, if any? We do have users requesting for 2005 support. While Linux (RHEL 7/8) has moved to supporting GB18030-2005 via glibc, Windows 10 and AIX 7.2 still have GB18030-2000 base. That means OpenJDK cannot move to GB18030-2005 base as yet. However, we can support both the versions until all the supported platforms move to GB18030-2005 base. Would that be an acceptable proposition? If we can have an enhancement request opened, I'd be glad to contribute the GB18030-2005 charset implementation. If I read this correctly, then your proposal is for GB18030 to continue to be GB18030-2000 but you would introduce a new charset GB18030 map to GB18030-2005 for the new version. Are you also proposing a system property or some means to have GB18030 be GB18030-2005 until the time is right to make it the default? -Alan
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v5]
On Wed, 10 Nov 2021 19:41:29 GMT, Jonathan Gibbons wrote: > 1. `PrintStream(OutputStream)` and `PrintStream(OutputStream, boolean)` > should be redefined so that they internally check if the stream arg is a > PrintStream, in which case they use the encoding from the `PrinStream` > instead of the default. I think you mean the PrintWriter constructors here. Yes, there is merit in that. PrintStream is a bit unusual in that it's an OutputStream but it has a charset because it prints text output. - PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v5]
On Wed, 10 Nov 2021 19:16:58 GMT, Jonathan Gibbons wrote: > Generally, the issue is related to the fact that we wrap a PrintStream in a > PrintWriter ... and, if I understand correctly, the writer ends up with the > wrong character encoding. Is it possible to change PrintWriter and/or > PrintStream so that the correct underlying encoding used by the PrintStream > is also used by the PrintWriter. That way, all existing uses where a > PrintWriter wraps a PrintStream would continue to work without any > modification. JEP 400 has moved the JDK to using UTF-8 as the default charset, a long overdue change. So if you create a PrintStream or a PrintWriter without specifying the charset then it will default to UTF-8. The issue that I think we have with javac and a few other tools is that they are creating PrintWriters on PrintStreams where the underlying streams are stdout/stderr and so using the native encoding. There was exploration into special casing this scenario during JEP 400 that was rejected because it complicates the spec and may not be feasible in cases where there are many layers in the onion. If there is resistance to fixing the tools then we might have to re-visit this. Naoto may have more to say on this. - PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: 8276348: Use blessed modifier order in java.base
On Tue, 2 Nov 2021 17:13:47 GMT, Roger Riggs wrote: > In comments, I think the 'synchronized static 'reads better, the conventional > order is for the code. I think Roger is right and maybe the change to the javadoc could be dropped from this patch. - PR: https://git.openjdk.java.net/jdk/pull/6213
Re: RFR: 8275863: Use encodeASCII for ASCII-compatible DoubleByte encodings
On Mon, 25 Oct 2021 10:16:23 GMT, Claes Redestad wrote: > Enhance ASCII-compatible `DoubleByte` encodings to make use of the > `StringCoding.implEncodeAsciiArray` intrinsic, which makes many such > `CharsetEncoder`s encode ASCII text at speeds comparable to most single-byte > encoders - and also more in line with how well these charsets behave when > calling `String.getBytes`: > > Before: > > Benchmark (size) (type) Mode Cnt Score Error > Units > CharsetEncodeDecode.encode 16384 ISO-8859-1 avgt 30 3.021 ± 0.120 > us/op > CharsetEncodeDecode.encode 16384 Shift-JIS avgt 30 47.793 ± 1.942 > us/op > CharsetEncodeDecode.encode 16384 GB2312 avgt 30 49.598 ± 2.006 > us/op > CharsetEncodeDecode.encode 16384 EUC-JP avgt 30 68.709 ± 5.019 > us/op > CharsetEncodeDecode.encode 16384 EUC-KR avgt 30 48.033 ± 1.651 > us/op > > > Patched: > > Benchmark (size) (type) Mode Cnt Score Error > Units > CharsetEncodeDecode.encode 16384 ISO-8859-1 avgt 30 2.856 ± 0.078 > us/op > CharsetEncodeDecode.encode 16384 Shift-JIS avgt 30 5.287 ± 0.209 > us/op > CharsetEncodeDecode.encode 16384 GB2312 avgt 30 5.490 ± 0.251 > us/op > CharsetEncodeDecode.encode 16384 EUC-JP avgt 30 7.657 ± 0.368 > us/op > CharsetEncodeDecode.encode 16384 EUC-KR avgt 30 5.718 ± 0.267 > us/op > > > The `isASCIICompatible` predicate on `DoubleByte` was added in JEP 254 for > the purpose of implementing such ASCII fast-paths, but is only used in what > is now `String.encodeWithEncoder` to speed up `String.getBytes(...)` > operations. > > Testing: tier1-3 Good work - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6102
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v3]
On Wed, 6 Oct 2021 05:04:28 GMT, Ichiroh Takiguchi wrote: >> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13. >> After JDK18-b13, javac and some other langtool command's usage were garbled >> on Japanese Windows. >> These commands use PrintWriter instead of standard out/err with PrintStream. > > Ichiroh Takiguchi has updated the pull request incrementally with one > additional commit since the last revision: > > 8274544: Langtools command's usage were garbled on Japanese Windows The changes in 4427d87c look okay. I assume most of these tools will never run with a SM enabled and don't need to be granted permission to reading native.encoding. - PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: 8272626: Avoid C-style array declarations in java.*
On Wed, 18 Aug 2021 10:07:35 GMT, Claes Redestad wrote: > C-style array declarations generate noisy warnings in IDEs, et.c. This patch > cleans up all java.* packages. > > (Copyrights intentionally not updated due the triviality of most changes) Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5161
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]
On Fri, 25 Jun 2021 23:40:27 GMT, Weijun Wang wrote: >> More refactoring to limit the scope of `@SuppressWarnings` annotations. >> >> Sometimes I introduce new methods. Please feel free to suggest method names >> you like to use. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > one more src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java line 53: > 51: private static final long CURRENT_PID = AccessController.doPrivileged( > 52: (PrivilegedAction) > ProcessHandle::current).pid(); > 53: The original code separated out the declaration of the PrivilegedAction to avoid this cast. If you move the code from the original static initializer into a static method that it called from initializer then it might provide you with a cleaner way to refactor this. There are several other places in this patch that could do with similar cleanup. - PR: https://git.openjdk.java.net/jdk17/pull/152
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v8]
On Tue, 1 Jun 2021 15:21:33 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 11 commits: > > - merge from master > - rename setSecurityManagerDirect to implSetSecurityManager > - default behavior reverted to allow > - move one annotation to new method > - merge from master, two files removed, one needs merge > - keep only one systemProperty tag > - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java > - feedback from Sean, Phil and Alan > - add supresswarnings annotations automatically > - manual change before automatic annotating > - ... and 1 more: > https://git.openjdk.java.net/jdk/compare/74b70a56...ea2c4b48 Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v6]
On Mon, 31 May 2021 15:02:57 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > default behavior reverted to allow System.setSecurityManagerDirect looks a bit ugly now. Can this be renamed to implSetSecurityManager and avoid the line break in the middle of the declaration? The usage of System.err usage in setSecurityManager also needs to be re-examined as this will run arbitrary code when System.err can be changed. To fix this will require capturing the stream at startup (as was done with the illegal access logger). It's okay to integrate with what you have for the first push and we can fix this issue with System.err when the warning message is changed to the intended message. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Fri, 21 May 2021 18:00:13 GMT, Phil Race wrote: > Are you suggesting that the patch doesn't need testing as it is ? It should > be the same either way. > It is very straight forward to run the automated tests across all platforms > these days. > I get the impression that no one is guaranteeing to do this straight after > integration. > It sounds like it is up for deferral if time runs out. > > The amount of follow-on work that I am hearing about here, and may be for > tests, does not make it sound > like this JEP is nearly as done as first presented. > > If there was some expectation that groups responsible for an area would get > involved with this > issue which I am assured was already known about, then why was it not raised > before and made > part of the plan ? Sprinkling SuppressWarnings should be very low risk. Refactoring code to have the scope of the SW be as small as possible may be a bit more risky, esp. in areas where one doesn't know the code or the tests that exercise it. The tests may be good but it's not clear to me that we want to force Max to do significant refactoring in this PR. PR 4138 has been created as the follow-on PR so I think we should help get that reviewed so that it can be integrated soon after this PR. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 14:52:21 GMT, Thiago Henrique Hüpner wrote: > IcedTea-Web seems to be using this method reflectively: > https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80 I assume this doesn't work with JDK 16, at least not without using --add-options to export jdk.internal.util.jar. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 13:13:04 GMT, Сергей Цыпанов wrote: > Hi guys, any more comments here? Please ping me if I've missed something I suspect this will require renaming some fields and changing comments, e.g. requestList is no longer a good name for the field in AbstractPoller, its comments need changes too. The field in ResolverConfigurationImpl is searchList, it will require a few changes. There may be more, I just picked out a few. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 04:22:32 GMT, Phil Race wrote: >> That is unfortunate, but nonetheless I think required to be done. >> We have acknowledeged this can't reasonably be called an RFE, so the JEP is >> introducing bugs/technical debt/call it what you will. This should generally >> be part of a sandbox for the JEP and fixed before integration of the JEP. >> From my point of view it is a blocker. > > The JEP isn't PTT for 17 so there's plenty of time isn't there ? There are 827 files in this patch. Phil is right that adding SW at the class level is introducing technical debt but if addressing that requires refactoring and re-testing of AWT or font code then it would be better to have someone from that area working on. Is there any reason why this can't be going on now on awt-dev/2d-dev and integrated immediately after JEP 411? I don't think we should put Max through the wringer here as there are too many areas to cover. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote:
> Please review this implementation of [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> The code change is divided into 3 commits. Please review them one by one.
>
> 1.
> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
> The essential change for this JEP, including the `@Deprecate` annotations
> and spec change. It also update the default value of the
> `java.security.manager` system property to "disallow", and necessary test
> change following this update.
> 2.
> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
> Manual changes to several files so that the next commit can be generated
> programatically.
> 3.
> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
> Automatic changes to other source files to avoid javac warnings on
> deprecation for removal
>
> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
> generated programmatically, see the comment below for more details. If you
> are only interested in a portion of the 3rd commit and would like to review
> it as a separate file, please comment here and I'll generate an individual
> webrev.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> any file to minimize unnecessary merge conflict.
>
> Furthermore, since the default value of `java.security.manager` system
> property is now "disallow", most of the tests calling
> `System.setSecurityManager()` need to launched with
> `-Djava.security.manager=allow`. This is covered in a different PR at
> https://github.com/openjdk/jdk/pull/4071.
Marked as reviewed by alanb (Reviewer).
src/java.base/share/classes/java/security/AccessController.java line 877:
> 875: @CallerSensitive
> 876: public static T doPrivileged(PrivilegedExceptionAction action,
> 877: @SuppressWarnings("removal")
> AccessControlContext context, Permission... perms)
you might find it easier if you put the Permissions parameter on a new line.
There are several places in AccessController where the same thing happens.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Tue, 18 May 2021 12:42:08 GMT, Sean Mullan wrote: >> src/java.base/share/classes/java/lang/SecurityManager.java line 315: >> >>> 313: * >>> 314: * @since 1.0 >>> 315: * @deprecated The Security Manager is deprecated and subject to >>> removal in a >> >> Javadoc will prefix, in bold, "Deprecated, for removal: This API element is >> subject to removal in a future version". I'm just wondering if the sentence >> will be repeated here, can you check? > > It includes both: >  Thanks for checking, I assumed that was the case so wondering if it should be dropped from the deprecation text to avoid the repeated sentence. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267110: Update java.util to use instanceof pattern variable
On Tue, 18 May 2021 10:37:21 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.util` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick You may need to coordinate with @DougLea on the changes to j.u.concurrent. - PR: https://git.openjdk.java.net/jdk/pull/4088
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal
On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang wrote: > Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. src/java.base/share/classes/java/lang/SecurityManager.java line 315: > 313: * > 314: * @since 1.0 > 315: * @deprecated The Security Manager is deprecated and subject to removal > in a Javadoc will prefix, in bold, "Deprecated, for removal: This API element is subject to removal in a future version". I'm just wondering if the sentence will be repeated here, can you check? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager
On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang wrote:
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas. Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
Marked as reviewed by alanb (Reviewer).
The changes look okay but a bit inconsistent on where -Djava...=allow is
inserted for tests that already set other system properties or other
parameters. Not a correctness issue, just looks odd in several places, e.g.
test/jdk/java/lang/System/LoggerFinder/BaseLoggerFinderTest/BaseLoggerFinderTest.java
- the tests sets the system properties after -Xbootclasspath/a: but the change
means it sets properties before and after.
test/jdk/java/lang/Class/getResource/ResourcesTest.java - you've added it in
the middle of the module and class path parameters.
For uses using ProcessTools then it seems to be very random.
-
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8266774: System property values for stdout/err on Windows UTF-8
On Fri, 7 May 2021 22:46:08 GMT, Naoto Sato wrote: > Please review this small fix to Windows system property init code. This is > leftover from the support for `Console.charset()` method, where it lacked to > initialize `sun.stdout/err.encoding` to `UTF-8` for the code page `cp65001`. > The fix has been manually verified, but no automated test case is provided, > as automatically setting `UTF-8` for the system locale on Windows test > machine seems not possible. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3931
Re: RFR: 8266155: Convert java.base to use Stream.toList()
On Wed, 28 Apr 2021 15:41:31 GMT, Chris Hegarty wrote: >> 8266155: Convert java.base to use Stream.toList() > > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6788: > >> 6786: } >> 6787: >> 6788: /** > > I think the import of Collectors can be dropped in this file? And maybe a few > other files too? The import can be dropped from the jdk.internal.module.* classes too. jdk.internal.module.IllegalAccessLogger will likely be removed as part of JEP 403. - PR: https://git.openjdk.java.net/jdk/pull/3734
Re: RFR: 8264544: Case-insensitive comparison issue with supplementary characters.
On Thu, 1 Apr 2021 03:24:04 GMT, Naoto Sato wrote: > Please review the fix to the subject issue. Thanks to the contribution by > Chris Johnson. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3300
Re: RFR: 8264332: Use the blessed modifier order in jdk.charsets
On Sun, 28 Mar 2021 13:56:00 GMT, Alex Blewitt wrote: > 8264332: Use the blessed modifier order in jdk.charsets Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3236
Re: RFR: 8189198: Add "forRemoval = true" to Applet API deprecations
On Wed, 17 Mar 2021 16:44:19 GMT, Andy Herrick wrote: > I cannot find any instances where the scope can be narrowed Are you about AquaInternalFrameUI.mouseRelased, BasicPopupMenuUI. stateChanged, and other non-trivial methods? - PR: https://git.openjdk.java.net/jdk/pull/2920
Re: RFR: 8263561: Re-examine uses of LinkedList
On Sun, 14 Mar 2021 17:26:07 GMT, Alan Bateman wrote: >> Looks like it's never specified in JavaDoc which particular implementation >> of List is used in fields of affected classes, so it's quite odd to me that >> someone would rely on that when using reflection. But your point about >> backward compatibility is reasonable, so I'll revert mentioned changes. > > Nothing outside of the JDK should be hacking into this private field of a > non-exposed class, this should not be a concern here. > @AlanBateman so is it ok to keep `ArrayLists`? One thing to look out for is usages of 2-arg add method that inserts at a specific position. This shouldn't be a concern in URLClassPath.closeLoaders (assuming this is this usage where the question arises). - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Sun, 14 Mar 2021 17:18:11 GMT, Сергей Цыпанов wrote: >> If that's the only use case, I recommend changing the return type to a >> deque, and replace the linked list with an array deque instead (as done >> elsewhere in this pr) > > Looks like it's never specified in JavaDoc which particular implementation of > List is used in fields of affected classes, so it's quite odd to me that > someone would rely on that when using reflection. But your point about > backward compatibility is reasonable, so I'll revert mentioned changes. Nothing outside of the JDK should be hacking into this private field of a non-exposed class, this should not be a concern here. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8189198: Add "forRemoval = true" to Applet API deprecations
On Sat, 13 Mar 2021 00:43:33 GMT, Alexander Matveev wrote: >> implementation of >> JDK-8256145: JEP 398: Deprecate the Applet API for Removal > > Marked as reviewed by almatvee (Committer). Have you looked at narrowing the use of the SuppressWarnings to local variable declarations to avoid adding it to some of these methods? - PR: https://git.openjdk.java.net/jdk/pull/2920
Re: RFR: JDK-8262471: Fix coding style in src/java.base/share/classes/java/lang/CharacterDataPrivateUse.java
On Fri, 26 Feb 2021 17:50:19 GMT, Christoph Göttschkes wrote: > Please review this small patch which fixes the coding style of > CharacterDataPrivateUse.java Looks fine. This source file was a .template until a few weeks ago, probably should have fixed the indentation issues when moving it to a .java file. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2754
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v2]
On Fri, 12 Feb 2021 04:06:55 GMT, Naoto Sato wrote: >> Please review this doc fix to j.l.Character, which now includes the table of >> the history of supported Unicode versions. A corresponding CSR will be filed >> accordingly. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Removed empty tag src/java.base/share/classes/java/lang/Character.java line 86: > 84: * Unicode 10.0 > 85: * Java SE 9 > 86: * Unicode 8.0 Do we really need the history in the API docs? Will will update this table if there is a MR of the JSR for Java 8 that moves to a new Unicode release? - PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 8261254: Initialize charset mapping data lazily
On Sun, 7 Feb 2021 19:08:18 GMT, Claes Redestad wrote: > This patch refactor JDK internal charsets to initialize charset mapping data > lazily when needed via holder classes. This means both a startup improvement > in some cases, and possible throughput improvements for all DoubleByte-based > Charsets. > > Testing: tier1-3 I wouldn't expect enumerating all charsets with Charset::availableCharsets to be too common but moving the data to holder class looks okay. The missing "final" in a few places was an oversight. The replacement of the foreach and method ref in getServicesCatalog with imperative code is disappointment but okay here. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2449
Re: RFR: 8260406: Do not copy pure java source code to gensrc
On Tue, 26 Jan 2021 10:35:03 GMT, Magnus Ihse Bursie wrote: > For java.base gensrc we do the following: > > # Copy two Java files that need no preprocessing. > $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/%.java: > $(CHARACTERDATA_TEMPLATES)/%.java.template > $(call LogInfo, Generating $(@F)) > $(call install-file) > > GENSRC_CHARACTERDATA += > $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/CharacterDataUndefined.java \ > > $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/CharacterDataPrivateUse.java > > What this means is that we have two "template" files that are just compiled > as java files, but only after being copied to gensrc. :-) > > We should just rename these files to java and move them to the normal source > directory. Good. I notice the comment in both source files says "Java.lang.Character" rather than "java.lang.Character", probably should fix that at some point. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2233
Re: RFR: 8257733: Move module-specific data from make to respective module [v4]
On Fri, 15 Jan 2021 14:58:14 GMT, Alan Bateman wrote: >> This PR is not stale; it's just still waiting for input from @AlanBateman. > > @magicus Can the CharacterDataXXX.template files move to > src/java.base/share/classes/java/lang? > @AlanBateman When I moved the charset templates, I found this gold nugget: > > ``` > # Copy two Java files that need no preprocessing. > $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/%.java: > $(CHARACTERDATA_TEMPLATES)/%.java.template > $(call LogInfo, Generating $(@F)) > $(call install-file) > > GENSRC_CHARACTERDATA += > $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/CharacterDataUndefined.java \ > > $(SUPPORT_OUTPUTDIR)/gensrc/java.base/java/lang/CharacterDataPrivateUse.java > ``` > > What this means is that we have two "template" files that are just compiled > as java files, but only after being copied to gensrc. :-) That definitely > needs cleaning up, but this PR is large enough as it is, so I will not do it > now. Good find, surprised this wasn't spotted before now. We should create a separate issue to rename them and get rid of the copying in the make file. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v4]
On Mon, 11 Jan 2021 09:20:07 GMT, Magnus Ihse Bursie wrote: >> Marked as reviewed by prr (Reviewer). > > This PR is not stale; it's just still waiting for input from @AlanBateman. @magicus Can the CharacterDataXXX.template files move to src/java.base/share/classes/java/lang? - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8226810: Failed to launch JVM because of NullPointerException occured on System.props
On Tue, 12 Jan 2021 16:26:27 GMT, Evan Whelan wrote: > Hi, > > Please review this small change which enables the GB18030 charset to be built > into java.base > > Thanks Including GB18030 in java.base rather than jdk.charsets on Windows is fine. It does increase the size of java.base but that is less of a concern now than it used to be. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2053
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 15 Dec 2020 22:52:30 GMT, Magnus Ihse Bursie wrote: >> Reviewed changes to `characterdata`, `charsetmapping`, `cldr`, `currency`, >> `lsrdata`, `tzdata`, and `unicodedata` with minor comment. Looks good >> overall. > > I think this is almost ready to be pushed, but I'd like to have someone from > the client team review the changes for java.desktop as well. @prrace Any > change you can have a look at this? I think it will be annoying to have the charset mapping tables and other data in the src tree, have you looked at other alternatives? - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8253497: Core Libs Terminology Refresh [v4]
On Tue, 15 Dec 2020 23:14:14 GMT, Brent Christian wrote:
>> This is part of an effort in the JDK to replace archaic/non-inclusive words
>> with more neutral terms (see JDK-8253315 for details).
>>
>> Here are the changes covering core libraries code and tests. Terms were
>> changed as follows:
>> 1. grandfathered -> legacy
>> 2. blacklist -> filter or reject
>> 3. whitelist -> allow or accept
>> 4. master -> coordinator
>> 5. slave -> worker
>>
>> Addressing similar issues in upstream 3rd party code is out of scope of this
>> PR. Such changes will be picked up from their upstream sources.
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> improve SERIAL_FILTER_PATTERN comment
Marked as reviewed by alanb (Reviewer).
src/java.base/share/classes/java/util/Locale.java line 1649:
> 1647: * This implements the 'Language-Tag' production of BCP47, and
> 1648: * so supports legacy (regular and irregular, referred to as
> 1649: * {@code Type: grandfathered} in BCP47) as well as
You might consider putting quotes around "Type; grandfathered" here, otherwise
looks good.
-
PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8253497: Core Libs Terminology Refresh [v2]
On Tue, 15 Dec 2020 01:46:08 GMT, Brent Christian wrote: >> This is part of an effort in the JDK to replace archaic/non-inclusive words >> with more neutral terms (see JDK-8253315 for details). >> >> Here are the changes covering core libraries code and tests. Terms were >> changed as follows: >> 1. grandfathered -> legacy >> 2. blacklist -> filter or reject >> 3. whitelist -> allow or accept >> 4. master -> coordinator >> 5. slave -> worker >> >> Addressing similar issues in upstream 3rd party code is out of scope of this >> PR. Such changes will be picked up from their upstream sources. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > updates, per code review test/jdk/java/nio/channels/SocketChannel/CloseRegisteredChannel.java line 45: > 43: SocketChannel client = SocketChannel.open (); > 44: client.connect (new InetSocketAddress > (InetAddress.getLoopbackAddress(), port)); > 45: SocketChannel channel = server.accept (); Can you rename this to "peer" instead? (we can remove the spurious space after accept while we are there). - PR: https://git.openjdk.java.net/jdk/pull/1771
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 12:15:38 GMT, Alan Bateman wrote: >> Also, to clarify, for me there is a fundamental difference between >> `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files >> that are part of the module, owned by the content team, and the `$MODULE` >> part is essential to delineate this content. The latter is owned by the >> build team, and is just a convenient way to organize the build system within >> the `make` directory. So it's clearly a no-no to put anything but `.gmk` >> files in `make/modules/$MODULE`. > > The mapping and nr tables, and the *-X.java.template files in > make/data/charsetmapping are used to generate source code for the java.base > and jdk.charsets modules. The stdcs-$OS files configure the package and > module that each charset go into. If the tables used to generate the source > files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk > will probably need a new home too. > @AlanBateman The process of modularization was not fully completed with > Project Jigsaw, and a few ugly warts remained. I was under the impression > that these should be addressed in follow-up fixes, but this was unfortunately > never done. Charsets and cldrconverter were both split between a core portion > in java.base and the rest in jdk.charsets and jdk.localedata, respectively, > but the split was never handled properly, but just "duct taped" in place. This is a complicated area of the build, not really a Project Jigsaw issue. It's complicated because the source code for the charsets is generated at build time and the set of non-standard charsets included in java.base varies by platform, e.g. there's are several IBMxxx charsets in java.base when building on AIX that are not interesting to include in java.base on other platforms. This means we can't split up the mapping tables in make/data/charsetmapping and put them in different directories. If you are moving them into the src tree then src/java.base (as you have it) is best but will still have the ugly wart that some of these mapping tables will be used to generate code for the jdk.charsets module. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 08:32:28 GMT, Magnus Ihse Bursie wrote: >> @mlchung If you don't have any strong preference, I implore you to accept >> this change. I **vastly** prefer to move the data out of `make`! >> >> This is not just about Skara tooling. When working with make files, autoconf >> and shell scripts, there is no fancy IDE support, so you are stuck with >> simple text editors and tools like `grep`. I've lost count on how many times >> I've had my grep searches blow up, since I happened to find e.g. a string in >> `tzdata` and get hundreds or more of hits. :-( And I do believe we will get >> a better code structure if the build team "owns" `make`; or at least has a >> vested interest in what's in that directory. We still suffer a lot of the >> old "I don't know where to put this file, so I'll just put it in make cause >> nobody cares about it anyway" mentality, but I've been working for quite >> some time to make that list of misplaced files shorter and shorter. > > Also, to clarify, for me there is a fundamental difference between > `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files > that are part of the module, owned by the content team, and the `$MODULE` > part is essential to delineate this content. The latter is owned by the build > team, and is just a convenient way to organize the build system within the > `make` directory. So it's clearly a no-no to put anything but `.gmk` files in > `make/modules/$MODULE`. The mapping and nr tables, and the *-X.java.template files in make/data/charsetmapping are used to generate source code for the java.base and jdk.charsets modules. The stdcs-$OS files configure the package and module that each charset go into. If the tables used to generate the source files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk will probably need a new home too. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module
On Fri, 4 Dec 2020 11:38:51 GMT, Magnus Ihse Bursie wrote: > And I can certainly move jdwp.spec to java.base instead. If jdwp.spec has to move to the src tree then src/java.se is probably the most suitable home because Java SE specifies JDWP as an optional interface. So nothing to do with java.base and the build will need to continue to generate the sources for the front-end (jdk.jdi) and back-end (jdk.jdwp.agent) as they implement the protocol. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module
On Fri, 4 Dec 2020 10:29:48 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. > > To facilitate review, here is a list on how the different directories under > make/data has moved. > > **To java.base:** > * blacklistedcertsconverter > * cacerts > * characterdata > * charsetmapping > * cldr > * currency > * lsrdata > * publicsuffixlist > * tzdata > * unicodedata > > **To java.desktop:** > * dtdbuilder > * fontconfig > * macosxicons > * x11wrappergen > > **To jdk.compiler:** > * symbols > > **To jdk.jdi:** > * jdwp > > **Remaining in make:** > * bundle > * docs-resources > * macosxsigning > * mainmanifest Are you proposing any text or guidelines to be added to JEP 201 as part of this? I think the location of jdwp.spec and its location in the build tree may need to be looked at again. It was convenient to have it in the make tree, from which the protocol spec, and source code for the front end (module jdk.jdi) and a header file for the back end (module jdk.jdwp.agent) are created. Given that the JDWP protocol is standard (not JDK-specific) then there may be an argument to put it in src/java.se instead of a JDK-specific module. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: JDK-8189198: Add "forRemoval = true" to Applet APIs [v2]
On Thu, 12 Nov 2020 20:48:13 GMT, Andy Herrick wrote: >> JDK-8189198: Add "forRemoval = true" to Applet APIs > > Andy Herrick has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - JDK-8189198: Add "forRemoval = true" to Applet APIs > - Merge branch 'master' into JDK-8189198 > - Merge branch 'master' into JDK-8189198 > - JDK-8189198: Add "forRemoval = true" to Applet APIs > - JDK-8189198: Add "forRemoval = true" to Applet APIs > - JDK-8189198: Add "forRemoval = true" to Applet APIs src/java.naming/share/classes/javax/naming/Context.java line 1087: > 1085: @Deprecated(since="16", forRemoval=true) > 1086: String APPLET = "java.naming.applet"; > 1087: }; Probably should be since="9" (the deprecation in JDK-8051422 pre-dates the enhanced deprecation work). - PR: https://git.openjdk.java.net/jdk/pull/1127
Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/814
Re: RFR: 8253208: Move CDS related code to a separate class
On Fri, 18 Sep 2020 23:47:56 GMT, Yumin Qi wrote: > With more CDS related code added to VM, it is time to move CDS code to a > separate class. CDS is the new class which is > specific to CDS. > Tests: tier1-4 src/java.base/share/classes/jdk/internal/misc/CDS.java line 42: > 40: public static native void defineArchivedModules(ClassLoader > platformLoader, ClassLoader systemLoader); > 41: > 42: public static native long getRandomSeedForCDSDump(); The moving of the archive methods to CDS looks okay but inconsistent to only comment 3 of the 5 methods. - PR: https://git.openjdk.java.net/jdk/pull/261
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 13:53:10 GMT, David Holmes wrote: >> @dholmes-ora raises a good point. Hopefully there is a balance point between >> a dozen different bugs / pull requests >> each targeting one area and one bug / PR targeting a dozen separate areas. I >> don't have a good general rule to suggest. >> Maybe @AlanBateman or @jddarcy can offer some advice? > > 14 cc'd mailing lists is unworkable. You need to be subscribed to lists to > post, but even if you are a reply-all will > be delayed due to all of the mails being held up for moderator approval due > to: > " Too many recipients to the message" > > I have a longer email coming once it gets through the moderator approval > delay. :( Patches that do global replace are always awkward. In this case, there are 150 files changed and most seem to be tests or changes to tools used in the build (includes src/hotspot/share/prims/jvmtiEnvFill.java). If these are dropped from the patch then it leaves just 43 files, many of which are from 3rd party code that can also be dropped. This should reduce the patch down to a manageable 24 or so files that should be trivial to review. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 08:47:35 GMT, Dmitriy Dumanskiy wrote: >> Before this Enhancement can be formally reviewed, you will need a JBS bug >> ID. If you are already working with a >> Committer or Reviewer in the `jdk` project who has agreed to sponsor your >> change, they can file the Enhancement >> request. Otherwise, you can file it using the web interface at >> [bugreport.java.com](https://bugreport.java.com/). Once >> you have a JBS bug ID, you need to edit the PR title to include that bug ID >> (without the `JDK-`) replacing >> "Improvement". Since this PR cuts across many functional areas (each gray >> label represents a functional area), you >> should expect a longer review process, since someone from each functional >> area will need to look at the changes in >> their area (like @mrserb started to do for the `2d` area). > > @kevinrushforth thanks. Done. > > Similar issues: > https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8215014 > https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8251246 > https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8223237 > > could be joined somehow? The code in java.base was updated to use String::isEmpty in JDK 12 (JDK-8215281). There was follow-up in JDK 13 to do the same in the java.desktop module (JDK-8223237). Changing the remaining usages make sense although I see that more more than half are in tests. It would be good to hear from security-dev on the changes to the Apache Santuario code (in java.xml.crypto module) in case it would be better to contribute those upstream instead. Ditto for the Apache Xalan code (in the java.xml module) but it may be significantly forked already that it doesn't matter. I assume you can use JDK-8215014 rather than creating a new issue. - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package [v2]
On Wed, 9 Sep 2020 09:49:44 GMT, Philippe Marschall wrote: >> Hello, newbie here >> >> I picked JDK-8138732 to work on because it has a "starter" label and I >> believe I understand what to do. >> >> - I tried to update the copyright year to 2020 in every file. >> - I decided to change `@since` from 9 to 16 since it is a new annotation >> name in a new package. >> - I tried to keep code changes to a minimum, eg not change to imports if >> fully qualified class names are used instead of >> imports. In some cases I did minor reordering of imports to keep them >> sorted alphabetically. >> - All tier1 tests pass. >> - One jpackage/jlink tier2 test fails but I don't believe it is related. >> - Some tier3 Swing tests fail but I don't think they are related. > > Philippe Marschall has refreshed the contents of this pull request, and > previous commits have been removed. The > incremental views will show differences compared to the previous content of > the PR. The pull request contains one new > commit since the last revision: > 8138732: Rename HotSpotIntrinsicCandidate to IntrinsicCandidate Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/45
Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package
On Mon, 7 Sep 2020 09:44:09 GMT, Philippe Marschall wrote: > Hello, newbie here > > I picked JDK-8138732 to work on because it has a "starter" label and I > believe I understand what to do. > > - I tried to update the copyright year to 2020 in every file. > - I decided to change `@since` from 9 to 16 since it is a new annotation name > in a new package. > - I tried to keep code changes to a minimum, eg not change to imports if > fully qualified class names are used instead of > imports. In some cases I did minor reordering of imports to keep them > sorted alphabetically. > - All tier1 tests pass. > - One jpackage/jlink tier2 test fails but I don't believe it is related. > - Some tier3 Swing tests fail but I don't think they are related. This one probably needs discussion on hotspot-dev to get agreement on the rename/move. It might need coordination with Graal. If the change does go ahead then please check if java.base's module-info.java still needs to export jdk.internal to jdk.jfr, I suspect that qualified export can be removed. - PR: https://git.openjdk.java.net/jdk/pull/45
Re: [14] RFR (XXS) 8238605: Correct the CLDR version number in cldr.md files
On 06/02/2020 16:50, naoto.s...@oracle.com wrote: Hello, Please review this extra small fix for this issue: https://bugs.openjdk.java.net/browse/JDK-8238605 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8238605/webrev.00/ It is simply updating the version number in cldr.md files, which should have been done with JDK-8231273. This seems to be a "must-fix" to the license file so I think it's okay. -Alan
Re: RFR: 8232161 Unexpected 1-way trip conversion entries on MS950 charset
On 14/10/2019 16:53, Ichiroh Takiguchi wrote: Hello. Could you review the fix ? Bug: https://bugs.openjdk.java.net/browse/JDK-8232161 Change: https://cr.openjdk.java.net/~itakiguchi/8232161/webrev.00/ I have a concern about 1-way trip conversion entries (4 entries) on MS950 charset. The detail information is in JDK-8232161 [1] Do you know any sense on the compatibility impact of changing this? I think Naoto has the same question and we aren't sure if this one with need a compatibility property. I think it will need a CSR. -Alan
Re: Fwd: RFR: JDK-8220414 :Correct copyright headers in Norm2AllModes.java and Normalizer2.java
Looks good. On 11/03/2019 10:06, Rachna Goel wrote: Hi, Kindly review fix to JDK-8220414, which updates copyright header. Bug: https://bugs.openjdk.java.net/browse/JDK-8220414 diff -r 645ba889ee5f src/java.base/share/classes/sun/text/normalizer/Norm2AllModes.java --- a/src/java.base/share/classes/sun/text/normalizer/Norm2AllModes.java Mon Jan 28 16:42:23 2019 +0100 +++ b/src/java.base/share/classes/sun/text/normalizer/Norm2AllModes.java Mon Mar 11 13:02:59 2019 +0530 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2018 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it diff -r 645ba889ee5f src/java.base/share/classes/sun/text/normalizer/Normalizer2.java --- a/src/java.base/share/classes/sun/text/normalizer/Normalizer2.java Mon Jan 28 16:42:23 2019 +0100 +++ b/src/java.base/share/classes/sun/text/normalizer/Normalizer2.java Mon Mar 11 13:02:59 2019 +0530 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2018 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it
Re: [12] RFR: 8215303: Allowing additional currency code points from later Unicode updates
On 04/01/2019 17:18, Chris Hegarty wrote:
Will compilations with `--release N-1` wind back the set of allowable
identifiers? It possibly should, if so how does one do similar when/if
the set of currency characters is expanded in an update release?
I don't think `javac --release` takes the Unicode version into account.
Using JDK 11 javac, I can compile the following with `--release 8` and
it will compile even though the Bitcoin currency symbol was only added
in Unicode 10 and Java SE 11. It won't compile with JDK 10 or older of
course.
class X {
double ₿ = 0.0;
}
I agree with your comment that the CSR could be be expanded to at least
make it clear that if support for a currency symbol is added in some
future update of JDK N then source code using it in identifiers will
compile with the JDK update that supports the symbol, not by the GA or
previous updates of JDK N.
-Alan
Re: Proposal: Unicode Variation Selector
This is the font code so I think you'll need to discuss it on 2d-dev at least. -Alan On 09/04/2018 04:57, Toshio 5 Nakamura wrote: Hello IBM would like to propose Unicode Variation Selector[1] capability to AWT and Swing components. This proposal is changing the following files: src/java.desktop/share/classes/sun/font/CMap.java src/java.desktop/share/classes/sun/font/CharToGlyphMapper.java src/java.desktop/share/classes/sun/font/CompositeGlyphMapper.java src/java.desktop/share/classes/sun/font/Font2D.java src/java.desktop/share/classes/sun/font/FontRunIterator.java src/java.desktop/share/classes/sun/font/FontUtilities.java src/java.desktop/share/classes/sun/font/TrueTypeGlyphMapper.java src/java.desktop/share/native/common/font/sunfontids.h src/java.desktop/share/native/libfontmanager/hb-jdk-font.cc src/java.desktop/share/native/libfontmanager/sunFont.c src/java.desktop/share/classes/javax/swing/text/DefaultEditorKit.java 542 lines will be changed. There are three parts. 1) Adding CMap format 14 support 2) Adding CharsToGlyphs functions support Variation Selector Sequences 3) Swing text component's DEL and BS key operations change How would I go about obtaining a sponsor? [1] http://www.unicode.org/versions/Unicode10.0.0/ch23.pdf Chapter 23.4 Variation Selectors Best regards, Toshio Nakamura IBM Japan
Re: [11] RFR: JDK-8198385: Remove property sun.locale.formatasdefault
On 21/02/2018 21:13, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8198385 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8198385/webrev.00/ The property was introduced in JDK7 for the backward compatibility, which at this point is no longer needed. Corresponding CSR is already approved. This looks okay. Are there are tests to be adjusted/removed as part of this? -Alan
Re: JDK 10 Review Request for JDK-8176583: Move currency data to /lib
On 09/06/2017 07:59, Nishit Jain wrote: Hi, Please review the fix for JDK-8176583 Bug: https://bugs.openjdk.java.net/browse/JDK-8176583 Webrev: http://cr.openjdk.java.net/~nishjain/8176583/webrev.01/ Fix: Relocated currency.data from java.base module (/java.base/java/util/currency.data) to /lib directory Can you explain why this is needed? What is planning to upgrade the currency data in generated run-time images? -Alan
Re: [9] RFR: 8171189: Deprecate ResourceBundleControlProvider for removal
On 06/01/2017 09:13, Yoshito Umaoka wrote: Yes. See https://github.com/IBM-Bluemix/gp-java-client#using-resourcebundlecontrolprovider-spi-java-8-or-later The jar file contains java.util.spi.ResourceBundleControlProvider [https://github.com/IBM-Bluemix/gp-java-client/blob/master/src/main/resources/META-INF/services/java.util.spi.ResourceBundleControlProvider], so a consumer of this library just need to drop the jar in the Java's extension directory. We suggest people to take this approach, because it does not require existing code changes at all (that means, they can easily enable/disable the extended feature with no source code changes). Of course, the library works fine if the consumer of this library explicitly specify the ResourceBundleControl implementation, but such approach does not work well if resource bundles are consumed indirectly. The extension mechanism has been removed in JDK 9 (since late 2014). The details are in JEP 220 [1]. More on this in the MR for JSR 337 [2] (the JSR for Java SE 8) where the deprecation of this "feature" was announced. See also the draft EDR for JSR 379 [3] (the JSR for Java SE 9) where the extension mechanism is listed for removal in Java SE 9. In general then I don't think we've seen much use of the extension mechanism in recent time. In particular the practice of dropping JAR files into the JDK ext directory has always been problematic when switching/upgrading JDK versions. We have seen a few cases where servers are configured to run with -Djava.ext.dirs=... but not many. Since 2014 then I'm only aware of three complaints because servers won't start when they have -Djava.ext.dirs=... on the command line. In two of these cases then the directory was empty, meaning there were no extensions so the product dropping the property. To help catch these usages (all part of the planning to remove this feature) then JDK 8 intrdouced the -XX:+CheckEndorsedAndExtDirs option to fail at startup if the property is set or there are extensions installed. I hope you can find an alternative for the usage here, maybe the resources can be deployed on the class path instead. -Alan [1] http://openjdk.java.net/jeps/220 [2] https://jcp.org/en/jsr/detail?id=337 [3] http://mail.openjdk.java.net/pipermail/java-se-9-spec-experts/2016-December/02.html
Re: RFR:JDK-8168906-Tighten permissions granted to the jdk.localedata module
On 14/11/2016 07:12, Rachna Goel wrote: Hi, Kindly review fix for JDK-8168906. https://bugs.openjdk.java.net/browse/JDK-8168906 Patch :http://cr.openjdk.java.net/~rgoel/JDK-8168906/webrev/ fix: As jdk.localedata module does read any system properties, tightened permissions for same. If you are sure it doesn't read any properties (or call anything that read properties without wrapping it in a privileged block) then it looks good. -Alan
Re: RFR: 8161203: ResourceBundle.getBundle performance regression
On 22/07/2016 14:13, Peter Levart wrote: : The changes are very straightforward. They preserve the general structure of the logic. I renamed the CacheKey nested class to LoadSession as it now only functions as a mutable object passed around the methods while executing the getBundle() process. LoadSession is now a factory for ClassLoaderValue cache key. I moved the loadTime and expirationTime fields from LoadSession (old CacheKey) to ResourceBundle. As each cached entry must maintain it's own loadTime/expirationTime, I changed the NONEXISTENT_BUNDLE constant into a private subclass of ResourceBundle. The back-link from ResourceBundle to CacheKey is not needed any more. There is a backlink from BundleReference to ClassLoaderValue key and the associated ClassLoader to enable expunging. Not a comment on the the ResourceBundle changes but I think it would be generally useful if we moved CLV jdk.internal. so that it can be used by other code in java.base. I had cause to do this recently when prototyping something in a completely different area recently. -Alan.
Re: RFR: 8161203: ResourceBundle.getBundle performance regression
On 21/07/2016 05:14, Masayoshi Okutsu wrote: Hi, Please review the fix for JDK-8161203. The fix is to lazily load ResourceBundleProviders. It's not necessary to load providers before cache look-up. Issue: https://bugs.openjdk.java.net/browse/JDK-8161203 Webrev: http://cr.openjdk.java.net/~okutsu/9/8161203/webrev.01 Thanks, Masayoshi This change looks okay to me. In passing, I can't quite tell if using CacheKey::providers is safe or not - this may be something to look into. -Alan
Re: RFR: 8159766: "Switching encoding from UTF-8 to ISO-8859-1" log message should be trace/debug message
On 21/06/2016 07:34, Masayoshi Okutsu wrote: Hi, Please review the fix for JDK-8159766. The logging code has been removed after all. No additional regression test. The message should no longer be logged to the .jtr file with java/util/ResourceBundle/UTF8Properties/CodePointTest.java. Issue: https://bugs.openjdk.java.net/browse/JDK-8159766 Wevrev: http://cr.openjdk.java.net/~okutsu/9/8159766/webrev.00 This looks okay to me. -Alan
Re: RFR: 8158272 & 8158468 (tools/jlink/plugins/IncludeLocalesPluginTest.java bug fixes)
On 13/06/2016 16:02, Mandy Chung wrote: I see. I’m fine with what you have. We should enhance the jlink testlibrary to run java from a run-time image created for a test to run. I'm okay with it too. -Alan
Re: RFR: 8158272 & 8158468 (tools/jlink/plugins/IncludeLocalesPluginTest.java bug fixes)
On 10/06/2016 08:08, Masayoshi Okutsu wrote: (re-sending to include jigsaw-dev) Hi, Please review fixes for 8158272 and 8158468. The test had several problems. - A child process doesn't inherit IO. Any outputs from the child process are not logged. - The exit code of a child process is ignored. The exit code needs to be checked by the test. - A child process should use the exit code to report errors rather than throwing a RuntimeException. - The golden data doesn't match the CLDR V29. - It may not be a good idea to hard-code the full set of the available locales. All have been fixed. Additional changes are: - Removed -verbose:gc from @run - Changed String to List for the available locales data. It was hard to compare long strings. - Changed output of the test so that it's easier to understand test activities. - Replaced Locale.ROOT.toString() with "(root)" As for the timeout issue, what happened is that a child process for checking available locales threw a RuntimeException due to the CLDR V29 changes. But the parent process (the main test process) was waiting for the child process to terminate. I'm not sure if it's a Windows specific process management issue or jtreg specific issue or something else. But the timeout symptom is no longer reproducible with the change to call ProcessBuilder.inheritIO(). Issues: https://bugs.openjdk.java.net/browse/JDK-8158272 https://bugs.openjdk.java.net/browse/JDK-8158468 Webrev: http://cr.openjdk.java.net/~okutsu/9/8158272.8158468/webrev.00 While I was working on these test problems, I noticed there are a few problems in the include locales plugin per se. For example, --include-locales=*-IN selects en-001 which is missing in the available locales list. (The missing "en-001" has been added to the test data, but it's commented out.) I will be filing a separate JBS issue. This looks quite good. What would you think also changing testAvailableLocales to use ProcessTools. As it stands then it looks like the output/error streams from the child won't be printed to the test logs? -Alan
Re: Review request: JDK-8158604: test/java/util/ResourceBundle/modules/appbasic missing @test
On 03/06/2016 05:09, Mandy Chung wrote: Masayoshi, test/java/util/ResourceBundle/modules/appbasic is not run because appbasic.sh was missed. I notice it while looking at the existing tests. I took the opportunity to improve the javadoc and update appbasic test to serve as a simple example of AbstractResourceBundleProvider. http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8158604/ Looks good except ResourceBundleProvider.java L33 where I assume the full spot has been removed so two sentences run into each other. -Alan
Re: RFR: 8031145: Re-examine closed i18n tests to see it they can be moved to the jdk repository.
On 26/05/2016 11:12, Masayoshi Okutsu wrote: On 5/26/2016 7:07 PM, Alan Bateman wrote: On 26/05/2016 10:41, Masayoshi Okutsu wrote: Naoto pointed out that test/java/text/Format/common/*Format.props should have the copyright header. Unfortunately, test/java/text/Format/common/PParser.java, which parses the .props files, doesn't support any comment lines. So, PParser has been changed to be able to support comment lines. test/java/text/Format/common/FormatIteratorTest.java, PParser.java, and *Format.props have been changed. No other changes. Webrev: http://cr.openjdk.java.net/~okutsu/9/8031145/webrev.02 Can the formatting/indentation in PParser be cleaned up before this is pushed? OK. I will. Thanks, otherwise I think this looks good and it's nice to get these tests moved to the jdk repo. -Alan
Re: RFR: 8031145: Re-examine closed i18n tests to see it they can be moved to the jdk repository.
On 26/05/2016 10:41, Masayoshi Okutsu wrote: Naoto pointed out that test/java/text/Format/common/*Format.props should have the copyright header. Unfortunately, test/java/text/Format/common/PParser.java, which parses the .props files, doesn't support any comment lines. So, PParser has been changed to be able to support comment lines. test/java/text/Format/common/FormatIteratorTest.java, PParser.java, and *Format.props have been changed. No other changes. Webrev: http://cr.openjdk.java.net/~okutsu/9/8031145/webrev.02 Can the formatting/indentation in PParser be cleaned up before this is pushed? -Alan
Re: RFR: 8031145: Re-examine closed i18n tests to see it they can be moved to the jdk repository.
On 23/05/2016 07:11, Masayoshi Okutsu wrote: Hi, Please review changes for JDK-8031145 that is to open closed i18n tests. There are many old tests which should be cleaned up. I did some, but there are still many. Issue: https://bugs.openjdk.java.net/browse/JDK-8031145 Webrev: http://cr.openjdk.java.net/~okutsu/9/8031145/webrev.00/ Liberating these tests is great and this looks like good work. Is there anything that we can do with the binary files? In the case of the .ser file then could it be a byte[] in the test with an execution mode that re-generates it? There is also at least one .data file that looks to be the serialized version of a bad ChoiceFormat and maybe that could be moved into the source too. -Alan
Re: RFR: 8153836: java/util/ResourceBundle/Bug6299235Test.sh depends on java.desktop
This looks okay to me. On 11/04/2016 08:36, Masayoshi Okutsu wrote: Hi all, Please review the fix for JDK-8153836. Issue: https://bugs.openjdk.java.net/browse/JDK-8153836 Webrev: http://cr.openjdk.java.net/~okutsu/9/8153836/webrev.00/ Thanks, Masayoshi
Re: RFR: 8152817: Locale data loading fails silently when running with a security manager
On 31/03/2016 10:29, Masayoshi Okutsu wrote: Webrev has been updated. I realized that some code which was for loading resource bundles in both java.base and jdk.localedata remained. The providers are no longer used for loading resource bundles in java.base. The code was removed. http://cr.openjdk.java.net/~okutsu/9/8152817/webrev.01/ This looks okay to me. -Alan
Re: [9] RFR: 8148346: Reduce number of packages in jdk.localedata module
Naoto - I don't know if this is hg version or webrev related but the moves aren't showing up correctly in the webrev so it's hard to see the actual changes. Can you try a new recent webrev to see if that fixes it? -Alan On 17/02/2016 21:28, Naoto Sato wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8148346 The proposed change is located at: http://cr.openjdk.java.net/~naoto/8148346/webrev.00/ Vast majority of the changes are changing package names. They used to be lang/ctry structure, which bloats the number of locale data package names to hundreds. Now it comes down to 8 packages with this fix. Naoto
Re: i18n dev Review Request for 8074431: Remove native2ascii tool
On 18/05/2015 23:45, Mandy Chung wrote: This patch removes native2ascii command-line tool from JDK 9 as proposed in March [1]. Looks good. -Alan
Re: i18n dev [9] RFR: 8061382: Separate CLDR locale data from JRE locale data
On 22/10/2014 20:52, Naoto Sato wrote: Hi Mandy, As I wrote in a separate email, my preference is the following module names: jdk.localedata.jre jdk.localedata.cldr This way, they both come under localedata (btw, they don't provide Locale, but the data for locale sensitive services such as DateFormat, so I prefer to keep localedata here), and jre/cldr are provider names which correspond to names in the system property. jdk.localedata.XXX looks right. I wonder if we can find something better than jre for the suffix of the first one. I ask because linking that into a runtime that isn't the what we know today as the JRE might be confusing. -Alan.
Re: i18n dev [9] RFR: 8061382: Separate CLDR locale data from JRE locale data
On 21/10/2014 01:25, Naoto Sato wrote: I think we can rename the original jdk.localedata to jdk.localedata.jre solely for the legacy JRE locale data, and create the new jdk.localedata.cldr. Or re-purpose jdk.localedata to represent the legacy JRE locale data, and newly create jdk.cldrlocaledata for the CLDR data. Either way they won't suggest the augmentation you refer. Do you mind putting this change on hold to allow for a discussion on the module naming? -Alan
Re: i18n dev [9] RFR: 8057646: ClassCircularityError running SQE test
On 09/09/2014 00:04, Naoto Sato wrote: Hello, Please review the fix for the following bug: https://bugs.openjdk.java.net/browse/JDK-8057646 The proposed changes are located at: http://cr.openjdk.java.net/~naoto/8057646/webrev.0/ It's not clear to me that this is the right place to address this issue. Clearly the changes to JRELocaleProviderAdapter to use ServiceLoader have upset the ordering that things are initialized for this error case, but it looks to me that it's always been very fragile. I think we need stack back a bit and examine the stack trace to see where might be a more appropriate place to address this, it may be that the security code needs to be more tolerant of attempts at recursive initialization. -Alan.
Re: i18n dev [9] RFR: 8057646: ClassCircularityError running SQE test
On 09/09/2014 18:14, Naoto Sato wrote: It's an inherent issue where some init code issues locale sensitive services, such as in this case, *Formatter. So this could happen not only in Security class, but could be anywhere. So I think we still need the change proposed here in order for avoiding regressions. On error cases, the diagnosability will be addressed with the issue with 8057075. I think we should move this issue to security-libs and have this issue addressed in the PolicyFile/PolicyParser code. My concern with catching Throwable is that it's just going to make it harder to diagnose tricky issues like this. I would be interested in other possible cases where you think the ClassCircularityError might happen. I'm just wondering if we need to approach this issue a bit differently. -Alan
Re: i18n dev [9] RFR 8038436: Re-examine the mechanism to determine available localedata and cldrdata
On 29/08/2014 22:07, Naoto Sato wrote: I incorporated the suggestions from Mandy and Alan. Also one change since the last webrev was to revert the hard-coding of the supported locales list back to the one which dynamically generates the lists at the build time. I initially thought static listing of locales would be less complex as to the build script/makefile, but on second thought it's less evil than possible future regressions. Please review: http://cr.openjdk.java.net/~naoto/8038436/webrev.5/ I think this looks okay and I assume you'll create another issue to re-examine the error handling as I do think that does need to be looked at again (my main concern is that it will silently failover and that could be very hard to diagnose). One comment on the test Bug8038436 is that it sets java.ext.dirs on the assumption that the locale and cldr data wouldn't be found. I suspect this will need to be re-worked once we get to the point of linking modules into the image. It may be that the ext directory is empty by default (localdata.jar and cldrdata.jar will not exist). -Alan.
Re: i18n dev [9] RFR 8038436: Re-examine the mechanism to determine available localedata and cldrdata
On 22/08/2014 19:46, Naoto Sato wrote:
Hello,
Please review the changes for the following issue:
https://bugs.openjdk.java.net/browse/JDK-8038436
The proposed changes are located at:
http://cr.openjdk.java.net/~naoto/8038436/webrev.3/
The idea is to introduce an SPI so that supported locales are
dynamically searched at runtime, not depending on the existence of
physical jar files.
I'd appreciate it if build folks could review the makefile related
changes, i18n forks to review locale framework files, and Mandy from
modularization point of view.
I've skimmed over the changes (not a detailed review, I see Mandy and
Masayoshi are doing that). It is good to see this code changed to use
ServiceLoader and dropping the direct access to localeddata.jar and
cldrdata.jar. It would be useful to see if you can run a few startup
performance tests to see that the changes have any impact.
On JREENLocaleDataMetaInfo then I wonder if you've considering using
camel case instead. Maybe JRE could be dropped (I realize the type is
JRE) and it can be renamed to EnLocaleDataMetaInfo to avoid all caps
JREEN.
A minor comment on JREENLocaleDataMetaInfo.getSupportedLocaleString is
that it could be changed to return
resourceNameToLocales.getOrDefault(resourceName, );
For the initialization then this might be help with some of the long
lines and might make the publication a bit clearer too:
private static final MapString, String resourceNameToLocales ;
static {
MapString, String map = new HashMap();
map.put(...);
resourceNameToLocales = map;
}
On the long lines then JRENonENLocaleDataMetaInfo will need attention as
the 872 character line will make future side-by-side reviews fun :-)
In CLDRLocaleProviderAdapter's constructor then it is catches/ignores
exceptions, the subsequent UOE does not help diagnose any issues.
Hopefully this can be re-examined before these changes are pushed as any
issue here would be difficult to diagnose.
One thing that isn't clear to me is the getLangTags implementations in
several classes where it has to cast to JRELocaleProviderAdapter. It's
not clear to the reader why this is necessary and how it would behave
with CLDRLocaleProviderAdapter.
-Alan.
Re: i18n dev [9] RFR: 8048123: Replace calendars.properties with another mechanism to specify a new Japanese calendar era
On 22/07/2014 08:06, Masayoshi Okutsu wrote:
Hello,
Please review the change for JDK-8048123. This change removes all the
era definitions in ${java.home}/lib/calendars.properties. (The
property file will eventually be gone later.) The major change is that
any new era of the Japanese calendar should now be defined using new
property jdk.calendar.japanese.supplemental.era.
https://bugs.openjdk.java.net/browse/JDK-8048123
Webrev includes some unrelated cleanups.
http://cr.openjdk.java.net/~okutsu/9/8048123/webrev.00/
I've skimmed over the changes (not a detailed review) and it's good to
have these properties dropped from calendars.properties. Clearly just
allowing for one additional era is a limitation and hopefully that will
not be an issue ever.
You might want to consider pre {@code ... } /pre instead of the
code tag. Alternatively it could be tables (although they might be
harder to maintain).
There are a couple of @SuppressWarnings values that I don't recognize,
are these warnings that javac emits?
Other minor comments in passing. The changes to
LocalGregorianCalendar.parseEraEntry highlight that it is still using
StringTokenizer, there might an opportunity to use String split here
instead. In getLocalGregorianCalendar you could use a lambda as has
been done in a few other places.
-Alan.
Re: i18n dev [9] RFR: 8048123: Replace calendars.properties with another mechanism to specify a new Japanese calendar era
On 22/07/2014 14:06, Masayoshi Okutsu wrote: : Other minor comments in passing. The changes to LocalGregorianCalendar.parseEraEntry highlight that it is still using StringTokenizer, there might an opportunity to use String split here instead. But split() uses regex, which I think is too expensive for this simple parsing. The parser wouldn't be used too often, though. String.split has a fast-path for simple usages like this. It's not core to what you are doing here of course, I only mentioned it because StringTokener is legacy. -Alan
Re: i18n dev [9] RFR: 8038092: Re-examine Bidi reflective dependency on java.awt.font
On 02/07/2014 16:43, Naoto Sato wrote: So I think the only question now is the test case and understanding why that needs to be updated. The test case ensures that even BidiBase class was loaded before TextAttribte/NumericShaper classes, it should work correctly. Version 1 of this fix (webrev.1) actually fails with this test case. Naoto Does it fail with the updated changes in webrev.2? I can't think why the test would need to be changed with the updated changes. -Alan
Re: i18n dev [9] RFR: 8038092: Re-examine Bidi reflective dependency on java.awt.font
On 03/07/2014 17:16, Naoto Sato wrote: With the original test case, webrev.1 and webrev.2 both succeed. With the modified test case (in webrev.2), webrev.1 fails but webrev.2 succeeds. The reason I changed the test case is to catch possible regression where someone makes changes to the SharedSecret being initialized at BidiBase class loading time (as in webrev.1). I'm not sure that I get this, it actually looks to me that this change to the test could mask a problem with a future change that would break the shared secrets setup. -Alan
Re: i18n dev [9] RFR: 8038092: Re-examine Bidi reflective dependency on java.awt.font
On 03/07/2014 17:59, Naoto Sato wrote: Ok, now I think I got what you mean. So it could regress in two ways, one is what you wrote below, and the other I am seeing. So instead of modifying the existing test case, I just add a new one which basically is the same one in the previous webrev as follows: http://cr.openjdk.java.net/~naoto/8038092/webrev.3/ This way they can detect those two possible regressions. Okay, I see where you are going. To avoid duplicating the testing then you should have the one test run twice (two @run lines). One run could be with a property set that you check in the static initializer so that you can decide whether to preload BidiBase. -Alan.
Re: i18n dev [9] RFR: 8038092: Re-examine Bidi reflective dependency on java.awt.font
On 02/07/2014 01:42, Naoto Sato wrote: I further modified the fix: - Made sure the SharedSecret is lazily evaluated. - Added the missing JavaAWTFontAccessImpl file - Added a test case http://cr.openjdk.java.net/~naoto/8038092/webrev.2/ Please review. This looks much better. I think the only case that need detailed examination now is whether it is possible for SharedSecrets.getJavaAWTFontAccess to return null. I'm thinking about the case where 2+ threads use Bidi for the first time, or say something else initializes TextAttribute at the same time that another thread uses Bidi for the first time. Is the change to Bug7051769.java really needed? -Alan
Re: i18n dev [9] RFR: 8038092: Re-examine Bidi reflective dependency on java.awt.font
On 02/07/2014 08:20, Alan Bateman wrote: : I think the only case that need detailed examination now is whether it is possible for SharedSecrets.getJavaAWTFontAccess to return null. I'm thinking about the case where 2+ threads use Bidi for the first time, or say something else initializes TextAttribute at the same time that another thread uses Bidi for the first time. On second look then I think this is okay because the static initializer must happen-before the use of the class. So I think the only question now is the test case and understanding why that needs to be updated. -Alan
Re: i18n dev [9] RFR: 8038092: Re-examine Bidi reflective dependency on java.awt.font
On 30/06/2014 18:35, Naoto Sato wrote: Hello, Please review the fix for the subject bug: https://bugs.openjdk.java.net/browse/JDK-8038092 The proposed change is located at: http://cr.openjdk.java.net/~naoto/8038092/webrev.0/ Thanks for looking at this issue. One part that doesn't look right is where Bidi is used before TextAttribute or NumericShaper are initialized and then used later with one of these as an attribute. Normally with SharedSecrets then ensureClassInitialized is to used to initialize a class that is known to register the secret but in this case then you can't do that because it would create a dependency on java.awt. The simplest thing might be to keep the Class.forName in both TextAttribtueConstants and NumericShapings as that will ensure that those classes are initialized (if they are present). A minor comment is that there are probably a bunch of imports that can be removed once the bulk of the core reflection usage goes away. -Alan.
Re: i18n dev [9] RFR: 8039317: Read currency.data read as a resource
On 26/06/2014 00:16, Naoto Sato wrote: Hello, Please review the proposed fix to the subject issue: https://bugs.openjdk.java.net/browse/JDK-8039317 The fix is to move currency.data file from lib directory into resources.jar file, as this file is merely a data file: http://cr.openjdk.java.net/~naoto/8039317/webrev.0/ I assume that jdk/make/profile-includes.txt will need to be updated too, otherwise it looks okay to me. -Alan.
Re: i18n dev RFR [9] 8043613: Update .properties files for serialver tool
On 04/06/2014 13:44, Chris Hegarty wrote: Michael, This is a very trivial request, and follows JDK-8042887 Remove serialver -show, this tool does not need a GUI. The only changes required are the removal of several values from the two localized property files. No additional translation is required. Won't this resolve this when the when translation drop happens? Normally we are not supposed to touch the translated properties files. The changes to these files were deliberately left out of Pavel's patch. -Alan.
