Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 12:37:48 GMT, Julian Waters wrote: >> SendaoYan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> delete extra empty trailing blank line in >> test/jdk/java/rmi/reliability/benchmark/bench/Makefile > > test/jdk/java/rmi/reliability/benchmark/bench/Makefile line 50: > >> 48: clean: >> 49: rm -f *.class .classes >> 50: > > Hmm, shouldn't this newline at EOF be kept? Asking @ all the people who've > reviewed this so far, no need to change it just yet No, it's an extra newline. A file should end with a newline but one is enough. - PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1631152127
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 07:29:39 GMT, SendaoYan wrote: >> Hi all, >> This PR several extra empty spaces and extra empty lines in several >> Makefiles. It's trivial fix, no risk. >> >> Thanks. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > delete extra empty trailing blank line in > test/jdk/java/rmi/reliability/benchmark/bench/Makefile Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19537#pullrequestreview-2104463763
Re: RFR: 8333477: Delete extra empty spaces in Makefiles
On Tue, 4 Jun 2024 07:47:46 GMT, SendaoYan wrote: > Hi all, > This PR several extra empty spaces and extra empty lines in several > Makefiles. It's trivial fix, no risk. > > Thanks. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19537#pullrequestreview-2096319123
Re: RFR: 8333301: Remove static builds using --enable-static-build
On Thu, 30 May 2024 19:14:43 GMT, Magnus Ihse Bursie wrote: > The original way of building static libraries in the JDK was to use the > configure argument --enable-static-build, which set the value of the make > variable STATIC_BUILD. (Note that this is not the same as the source code > definition STATIC_BUILD, which will be set by other means as well.) > > This method only ever worked on macOS, and has long since stopped working. It > was originally introduced for the Mobile Project, but I've now learn that not > even they use it anymore. > > It just adds clutter to the build system, and should be removed. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19487#pullrequestreview-2091321250
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with three > additional commits since the last revision: > > - Fix another typo > - Fix typo > - Add more comments Build changes look good. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107563120
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution
On Fri, 23 Feb 2024 16:23:43 GMT, Magnus Ihse Bursie wrote: > The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17986#pullrequestreview-1898688421
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck [v2]
On Fri, 16 Feb 2024 12:43:25 GMT, Magnus Ihse Bursie wrote: >> Since jcheck only checks file in a commit, there is a possibility of us >> getting files in the repository that would not be accepted by jcheck. This >> can happen when extending the set of files checked by jcheck, or if jcheck >> changes how it checks files (perhaps due to bug fixes). >> >> I have now run jcheck over the entire code base, and fixed a handful of >> issues. With these changes, jcheck accept the entire code base. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Replace spaces with \t in test properties file Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17871#pullrequestreview-1885177994
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 12:26:11 GMT, Magnus Ihse Bursie wrote: >> Since jcheck only checks file in a commit, there is a possibility of us >> getting files in the repository that would not be accepted by jcheck. This >> can happen when extending the set of files checked by jcheck, or if jcheck >> changes how it checks files (perhaps due to bug fixes). >> >> I have now run jcheck over the entire code base, and fixed a handful of >> issues. With these changes, jcheck accept the entire code base. > > test/jdk/java/util/Currency/currency.properties line 18: > >> 16: SB=EUR,111,2, 2099-01-01T00:00:00 >> 17: US=euR,978,2,2001-01-01T00:00:00 >> 18: ZZ = ZZZ , 999 , 3 > > This looks weird, but so did the original code. I assume this is to clearly > indicate this as a end of list marker. This looks weird indeed. Luckily it's just test code. Did you run the test after this change? - PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1491056108
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:42:02 GMT, Magnus Ihse Bursie wrote: >> This is an attempt to finally implement the idea brought forward in >> JDK-8295729: Properties files is essentially source code. It should have >> the same whitespace checks as all other source code, so we don't get >> spurious trailing whitespace changes or leading tabs instead of spaces. >> >> With Skara jcheck, it is possible to increase the coverage of the whitespace >> checks. >> >> However, this turned out to be problematic, since trailing whitespace is >> significant in properties files. That issue has mostly been sorted out in a >> series of PRs, and this patch will finish the job with the few remaining >> files, and actually enable the check in jcheck. > > src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/msg/DOMMessages.properties > line 24: > >> 22: >> 23: BadMessageKey = The error message corresponding to the message >> key can not be found. >> 24: FormatFailed = An internal error occurred while formatting the >> following message:\n > > At first glance, it might look like something should follow the `:`, but note > that there is a `\n` so if anything this will only make the next line > improperly indented. That could have been intentional though. Not sure if it was a good idea, but indenting something is a way of making something stand out as a quote. But looking at every use site, there is an extra space added anyway, so this seems fine. If we wanted to preserve the exact same behavior, all use sites should be updated to add 3 spaces. - PR Review Comment: https://git.openjdk.org/jdk/pull/17789#discussion_r1484416962
Re: RFR: 8322873: Duplicate -ljava -ljvm options for libinstrument
On Tue, 2 Jan 2024 21:44:04 GMT, Mikael Vidstedt wrote: > The libinstrument library is linked against libjava and libjvm, twice. This > is mostly harmless but Xcode 15.x generates a warning: > > Creating support/modules_libs/java.instrument/libinstrument.dylib from 12 > file(s) > ld: warning: ignoring duplicate libraries: '-ljava', '-ljvm' > > In particular, note that the `JDKLIB_LIBS` variable passed in to `LIBS` > already contains the -ljava -ljvm options. Digging through the history it > seems like this issue was introduced with > [JDK-8141290](https://bugs.openjdk.org/browse/JDK-8141290). > > Testing: tier1,builds-tier[2-5] Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17230#pullrequestreview-1802105849
Integrated: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 22:49:41 GMT, Erik Joelsson wrote: > There are a number of files in the `test` directory that have an incorrect > copyright header, which includes the "classpath" exception text. This patch > removes that text from all test files that I could find it in. I did this > using a combination of `sed` and `grep`. Reviewing this patch is probably > easier using the raw patch file or a suitable webrev format. > > It's my assumption that these headers were introduced by mistake as it's > quite easy to copy the wrong template when creating new files. This pull request has now been integrated. Changeset: 020255a7 Author:Erik Joelsson URL: https://git.openjdk.org/jdk/commit/020255a72dc374ba0bdd44772047f14a8bfe69a9 Stats: 1944 lines in 648 files changed: 0 ins; 1296 del; 648 mod 8267174: Many test files have the wrong Copyright header Reviewed-by: valeriep, aivanov, iris, dholmes, ihse - PR: https://git.openjdk.org/jdk/pull/15573
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 22:49:41 GMT, Erik Joelsson wrote: > There are a number of files in the `test` directory that have an incorrect > copyright header, which includes the "classpath" exception text. This patch > removes that text from all test files that I could find it in. I did this > using a combination of `sed` and `grep`. Reviewing this patch is probably > easier using the raw patch file or a suitable webrev format. > > It's my assumption that these headers were introduced by mistake as it's > quite easy to copy the wrong template when creating new files. Thanks for reviews! - PR Comment: https://git.openjdk.org/jdk/pull/15573#issuecomment-1716362585
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 23:12:51 GMT, Chris Plummer wrote: > I wonder if this is the right thing to do for the hprof files. I believe they > originated from some hprof tools that we no longer ship. 3rd parties might > choose to integrate them into their own tools. Do you think I should revert them? - PR Comment: https://git.openjdk.org/jdk/pull/15573#issuecomment-1708676439
RFR: 8267174: Many test files have the wrong Copyright header
There are a number of files in the `test` directory that have an incorrect copyright header, which includes the "classpath" exception text. This patch removes that text from all test files that I could find it in. I did this using a combination of `sed` and `grep`. Reviewing this patch is probably easier using the raw patch file or a suitable webrev format. It's my assumption that these headers were introduced by mistake as it's quite easy to copy the wrong template when creating new files. - Commit messages: - JDK-8267174 Changes: https://git.openjdk.org/jdk/pull/15573/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15573=00 Issue: https://bugs.openjdk.org/browse/JDK-8267174 Stats: 1944 lines in 648 files changed: 0 ins; 1296 del; 648 mod Patch: https://git.openjdk.org/jdk/pull/15573.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15573/head:pull/15573 PR: https://git.openjdk.org/jdk/pull/15573
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 07:41:11 GMT, David Holmes wrote: > This seems to run contrary to the requirement that files end in a newline, > which git will complain about if the newline is missing. The patch is leaving exactly one newline at the end of the file. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1612588091
Re: RFR: 8298047: Remove all non-significant trailing whitespace from properties files [v2]
On Sat, 24 Jun 2023 06:47:48 GMT, Vladimir Sitnikov wrote: > I've posted the suggetion to add .editorconfig on ide-support-dev, however, > the list does not seem to be active: > https://mail.openjdk.org/pipermail/ide-support-dev/2023-June/000281.html I think that sounds like a reasonable idea. I'm not following that mailing list. I think you can just post a PR with your suggested contents and target build-dev (through the `build` label). I'm not familiar with this kind of file, so an explanation of which IDEs support it would be good to include. - PR Comment: https://git.openjdk.org/jdk/pull/11491#issuecomment-1607073914
Re: RFR: JDK-8310550: Adjust references to rt.jar
On Wed, 21 Jun 2023 15:18:19 GMT, Matthias Baesken wrote: > There are a few references to rt.jar in comments and in the codebase itself. > Some of them might be removed or adjusted. The update to Java.gmk is good. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14593#pullrequestreview-1491263836
Re: RFR: 8308286 Fix clang warnings in linux code [v5]
On Sun, 11 Jun 2023 16:38:31 GMT, Artem Semenov wrote: >> When using the clang compiler to build OpenJDk on Linux, we encounter >> various "warnings as errors". >> They can be fixed with small changes. > > Artem Semenov has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - update > - update > - update > - update > - 8308286 Fix clang warnings in linux code Build changes look ok. - PR Comment: https://git.openjdk.org/jdk/pull/14033#issuecomment-1587242876
Re: RFR: 8307326: Package jdk.internal.classfile.java.lang.constant become obsolete
On Mon, 15 May 2023 08:38:54 GMT, Adam Sotona wrote: > Package `jdk.internal.classfile.java.lang.constant` containing `ModuleDesc` > and `PackageDesc` become obsolete after > [JDK-8306729](https://bugs.openjdk.org/browse/JDK-8306729). > All references to `jdk.internal.classfile.java.lang.constant.ModuleDesc` and > `jdk.internal.classfile.java.lang.constant.PackageDesc` across all JDK > sources, tests and JMH benchmarks are replaced with > `java.lang.constant.ModuleDesc` and `java.lang.constant.PackageDesc`. > `jdk.internal.classfile.java.lang.constant` package export hooks are removed > from java.base module-info, make files and test headers. > Content of `jdk.internal.classfile.java.lang.constant` package and related > tests under `jdk.classfile` are deleted. > Method references renamed in > [JDK-8306729](https://bugs.openjdk.org/browse/JDK-8306729) are fixed: > - `PackageDesc::packageName` to `PackageDesc::name` > - `PackageDesc::packageInternalName` to `PackageDesc::internalName` > - `ModuleDesc::moduleName` to `ModuleDesc::name`. > > Please review this pull request. > > Thanks, > Adam Build changes look ok. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13979#pullrequestreview-1426543240
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v15]
On Mon, 17 Apr 2023 20:59:06 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Merge branch 'master' into 8304915-arch-enum > - ArchTest on Debian RISC-V 64 confirmed by reviewer > - Fixed isPPC64(). >Consolidated switch cases in ArchTest. >Moved mapping of build TARGET_OS and TARGET_CPU to the build >to avoid multiple mappings in more than one place. > - Correct mapping and test of ppc64 > - Add ppc64 as mapping to PPC64 Architecture > - Modified test to check Architecture is64bits() and isLittleEndian() >against Unsafe respective values. >Relocated code mapping OS name and arch name from PlatformProps to >OperatingSystem and Architecture. Kept the mapping of names >in the template close to where the values are filled in by the build. > - Remove unused static and import of Stabile > - Rename OperatingSystemProps to PlatformProps. >Refactor OperatingSystem initialization to use strings instead of integers. > - Revised mapping mechanism of build target architecture names to enum > values. >Unrecognized values from the build are mapped to enum value "OTHER". >Renamed PPC64LE to PPC64 to reflect only the architecture, not the > endianness. >Added an `isLittleEndian` method to return the endianness (not currently > used anywhere) > - Revert changes to jdk.accessibility AccessBridge > - ... and 7 more: https://git.openjdk.org/jdk/compare/8858d543...99a93b7e Build changes look good. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13357#pullrequestreview-1390984858
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v13]
On Fri, 14 Apr 2023 14:31:31 GMT, Roger Riggs wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed isPPC64(). >> Consolidated switch cases in ArchTest. >> Moved mapping of build TARGET_OS and TARGET_CPU to the build >> to avoid multiple mappings in more than one place. > > make/modules/java.base/gensrc/GensrcMisc.gmk line 72: > >> 70: endif >> 71: >> 72: $(eval $(call SetupTextFileProcessing, BUILD_PLATFORMPROPERTIES_JAVA, \ > > @erikj79 Is there a better/good way to do these mappings, or select "local" > variable names? Not sure if it's better, but you could consider doing this directly in the switch statements in `make/autoconf/platform.m4` and add the corresponding variables in `spec.gmk.in`. That would make them available globally in the build, which may also be detrimental as parts of the build could start relying on them, and we end up with even more variants sprinkled around. I think what you have here is better for now. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1167095379
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply
On Wed, 5 Apr 2023 15:58:08 GMT, Roger Riggs wrote: > Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, > PPC64LE` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The values of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Looks good from a build point of view. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13357#pullrequestreview-1373301135
Re: RFR: 8296926: Use proper include lines for files in include/ [v2]
On Tue, 15 Nov 2022 10:42:58 GMT, Stefan Karlsson wrote: >> One of the more prevalent issues is that files in src/hotspot/share/include >> are not properly sorted. There has been some discussion that that was done >> on purpose, but it just adds another exception to the include rules that >> don't have any practical purposes, IMHO. It also goes against our written >> style guide around include files. One argument why it was OK have the files >> in include/ pushed up to the top of the sorted block, was that the file was >> included without specifying a directory. That's an argument that contradicts >> how we treat platform-dependent files, which (unfortunately) often also are >> specified without a prefixed directory. To remove this special case, I've >> removed the extraneous make file entry to have src/hotspot/share/include in >> the set of directories to search for headers when compiling HotSpot. Now all >> the header files in src/hotspot/share/include gets included by specifying >> the path from src/hotspot/share, just like the other platform-independent >> headers in HotSpo t. >> >> This RFE splits out the 'include/' changes from #11108 / JDK-8296886, so >> that those changes can be discussed separately. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Remove include/ from includes Build change looks good. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.org/jdk/pull/11133
Re: RFR: 8295729: Remove trailing whitespace from non-value lines in properties files [v2]
On Fri, 21 Oct 2022 08:17:46 GMT, Magnus Ihse Bursie wrote: >> Properties files is essentially source code. It should have the same >> whitespace checks as all other source code, so we don't get spurious >> trailing whitespace changes. >> >> With the new Skara jcheck, it is possible to increase the coverage of the >> whitespace checks (in the old mercurial version, this was more or less >> impossible). >> >> The only manual change is to `.jcheck/conf`. All other changes were made by >> running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ >> \t]*$//'`. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove check for .properties from jcheck > - Restore trailing whitespace for property values Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8295729: Add jcheck whitespace checking for properties files
On Thu, 20 Oct 2022 11:58:58 GMT, Magnus Ihse Bursie wrote: > Properties files is essentially source code. It should have the same > whitespace checks as all other source code, so we don't get spurious trailing > whitespace changes. > > With the new Skara jcheck, it is possible to increase the coverage of the > whitespace checks (in the old mercurial version, this was more or less > impossible). > > The only manual change is to `.jcheck/conf`. All other changes were made by > running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ > \t]*$//'`. Given that trailing whitespace may be part of a property value, then I take back my review. - Changes requested by erikj (Reviewer). PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8295715: Minimize disabled warnings in serviceability libs
On Thu, 20 Oct 2022 10:15:50 GMT, Magnus Ihse Bursie wrote: > After [JDK-8294281](https://bugs.openjdk.org/browse/JDK-8294281), it is now > possible to disable warnings for individual files instead for whole > libraries. I used this opportunity to go through all disabled warnings in the > serviceability native libraries. > > Any warnings that were only triggered in a few files were removed from the > library as a whole, and changed to be only disabled for those files. > > Some warnings didn't trigger in any file anymore, and could just be removed. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10786
Re: RFR: 8295729: Add jcheck whitespace checking for properties files
On Thu, 20 Oct 2022 11:58:58 GMT, Magnus Ihse Bursie wrote: > Properties files is essentially source code. It should have the same > whitespace checks as all other source code, so we don't get spurious trailing > whitespace changes. > > With the new Skara jcheck, it is possible to increase the coverage of the > whitespace checks (in the old mercurial version, this was more or less > impossible). > > The only manual change is to `.jcheck/conf`. All other changes were made by > running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ > \t]*$//'`. Nice job! - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.org/jdk/pull/10792
