Re: RFR (L) 8220747: Migrate data structures to being more C++

[David Holmes]

Hi Jc,

I just a had a quick look at this so not a full review - sorry.

I'm not sure it makes sense to define classes within "extern C {". The 
extern C is intended to define an interface for this C++ library to be 
used from a C program - as discussed here for your Solaris issue:


https://docs.oracle.com/cd/E18659_01/html/821-1383/bkamu.html

Cheers,
David

On 2/05/2019 1:08 pm, Jean Christophe Beyler wrote:

Hi all,

Re-sending with the full title

(OK... so JC will promise to go around the block 3 times before submitting
a review request; and will do any item you would like to redeem myself; I
apologize profusely and feel horrible...)

I want to move the libHeapMonitorTest.c to C++ and here is the first "step"
towards that. There are two parts to this: move the file to C++ and move
some of the C-style to C++-style code.

But this webrev failed on solaris; Igor helped me figure it out and his
solution was to add the change to the JtregNativeHotspot.gmk for solstudio.
We are not sure this is the "right" solution to this and hence have added
both the serviceability and build lists to review both file changes and
figure out what is best :)

This does pass the submit-repo testing and the tests on my local dev
machine.

Bug: https://bugs.openjdk.java.net/browse/JDK-8220747
Webrev: http://cr.openjdk.java.net/~jcbeyler/8220747/webrev.02/

Thanks all for your help,
Jc

RFR (L) 8220747: Migrate data structures to being more C++

[Jean Christophe Beyler]

Hi all,

Re-sending with the full title

(OK... so JC will promise to go around the block 3 times before submitting
a review request; and will do any item you would like to redeem myself; I
apologize profusely and feel horrible...)

I want to move the libHeapMonitorTest.c to C++ and here is the first "step"
towards that. There are two parts to this: move the file to C++ and move
some of the C-style to C++-style code.

But this webrev failed on solaris; Igor helped me figure it out and his
solution was to add the change to the JtregNativeHotspot.gmk for solstudio.
We are not sure this is the "right" solution to this and hence have added
both the serviceability and build lists to review both file changes and
figure out what is best :)

This does pass the submit-repo testing and the tests on my local dev
machine.

Bug: https://bugs.openjdk.java.net/browse/JDK-8220747
Webrev: http://cr.openjdk.java.net/~jcbeyler/8220747/webrev.02/

Thanks all for your help,
Jc

RFR (L) 8220747:

[Jean Christophe Beyler]

Hi all,

I want to move the libHeapMonitorTest.c to C++ and here is the first "step"
towards that. There are two parts to this: move the file to C++ and move
some of the C-style to C++-style code.

But this webrev failed on solaris; Igor helped me figure it out and his
solution was to add the change to the JtregNativeHotspot.gmk for solstudio.
We are not sure this is the "right" solution to this and hence have added
both the serviceability and build lists to review both file changes and
figure out what is best :)

This does pass the submit-repo testing and the tests on my local dev
machine.

Bug: https://bugs.openjdk.java.net/browse/JDK-8220747
Webrev: http://cr.openjdk.java.net/~jcbeyler/8220747/webrev.02/

Thanks all for your help,
Jc

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

I have filed JDK-8223189 
 to address these.


/Andy


On 4/30/2019 7:02 PM, Kevin Rushforth wrote:

I have a couple nit-picky comments:

1. The change to src/jdk.jlink/share/classes/module-info.java is 
unrelated to jpackage and should be reverted (there is only a 
white-space change and a copyright date change to that file)



2. The following files have whitespace errors that will cause jcheck 
to fail:
src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacAppImageBuilder.java:326: 
Trailing whitespace
src/jdk.jpackage/share/classes/jdk/jpackage/internal/CLIHelp.java:58: 
Tab character
src/jdk.jpackage/share/classes/jdk/jpackage/internal/JLinkBundlerHelper.java:217: 
Trailing whitespace
src/jdk.jpackage/share/classes/jdk/jpackage/internal/ValidOptions.java:55: 
Trailing whitespace



3. I recommend to replace the wild-card imports with explicit imports, 
for example:
src/jdk.jpackage/linux/classes/jdk/jpackage/internal/LinuxDebBundler.java 
(java.util.* and java.io.*)
(I think the wild-card static import is fine, just not the import 
every class from a package)


I'll try to remember to note these as I go through the review. This 
one could be done as a follow-up bug rather than doing it prior to 
integration if you prefer.


-- Kevin

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

I filed task JDK-8223187 
 to look into (1) and 
CR JDK-8223188  to 
address (2).


/Andy


On 4/30/2019 6:53 PM, Phil Race wrote:

A couple of questions / observations :-
1) setlocale
http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/linux/native/jpackageapplauncher/launcher.cpp.html

   52 int main(int argc, char *argv[]) {
   53 int result = 1;
   54 setlocale(LC_ALL, "en_US.utf8");

Why is this setlocale() call there ?

What does this mean for a user whose desktop is (say) German, or French, or 
Japanese ?

When the Java app is launched from this environment is it inheriting this US 
locale ? I hope not.

We have the same on Mac :-

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/macosx/native/jpackageapplauncher/main.m.html

and windows :-

http://cr.openjdk.java.net/~herrick/8212780/webrev.ea6/src/jdk.jpackage/windows/native/jpackageapplauncher/WinLauncher.cpp.html

   64 ::setlocale(LC_ALL, "en_US.utf8");


2) C++ files containing C

src/jdk.jpackage/windows/native/libjpackage/WindowsRegistry.cpp



src/jdk.jpackage/windows/native/libjpackage/jpackage.cpp



src/jdk.jpackage/windows/native/libwixhelper/libwixhelper.cpp



have their entire contents wrapped in

   36 #ifdef __cplusplus
   37 extern "C" {
   38 #endif

  159 #ifdef __cplusplus
  160 }
  161 #endif

wouldn't it be better to put them in .c files ?


-phil.