RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[JC Beyler]

Hi Igor,

I looked over the changes and tried to pay attention to all the mechanical
changes done. They look good to me as they are the step in the direction of
getting it all in C++. I will be happy to start on 8209547 once this goes
in.

(As always, not an official reviewer)

Two nits:

- We could use static_cast?
+        *new_bytes = (u1*) realloc(gen, *new_length);

- Add a space since we are doing the change (this is one example)?

-    arrayOrig[i]=env->GetObjectArrayElement(orig,i); CE
-    arrayClone[i]=env->GetObjectArrayElement(clone,i); CE
+    arrayOrig[i]=(jarray) env->GetObjectArrayElement(orig,i); CE
+    arrayClone[i]=(jarray) env->GetObjectArrayElement(clone,i); CE


On the other hand, we could just clean this up later once this big move to
C++ happens, so I'd be happy to see us tackle it in a second/third step.

So looks good to me and thanks for doing it!
Jc

On Tue, Aug 21, 2018 at 9:58 PM Igor Ignatyev <igor.ignatyev at oracle.com>
wrote:

> Hi David,
>
> thanks for looking at the webrev and all your comments. my answers are
> inlined.
>
> enjoy your vacation!
>
> -- Igor
>
> > On Aug 21, 2018, at 9:28 PM, David Holmes <david.holmes at oracle.com>
> wrote:
> >
> > Hi Igor,
> >
> > On 22/08/2018 9:04 AM, Igor Ignatyev wrote:
> >> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html <
> http://cr.openjdk.java.net/%7Eiignatyev//8209611/webrev.02/index.html> is
> a new version of patch, which moves only vmTestbase tests.
> >
> > This seems okay in principle. I didn't verify all the mechanical changes
> individually.
> >
> > make/common/TestFilesCompilation.gmk
> >
> > I suspect you can just find all .c and .cpp files and process them
> together, rather than duplicate all the logic. But I'll leave that to
> Magnus to comment on.
> I had to duplicate this foreach cycle to specify a correct TOOLCHAIN, w/o
> it linking fails on linux-slowdebug.
> >
> > make/test/JtregNativeHotspot.gmk
> >
> > + BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS := -D__STDC_FORMAT_MACROS
> -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
> >
> > Just a note that defining these is obsolete once we use C11/C++11 -
> which I think is an intent for JDK 12 if possible.
> initially I wanted to use <cinttypes> instead of <inttype.h> +
> -D__STDC_FORMAT_MACROS, but solaris studio apparently doesn't have
> cinttypes header file.
>
> hotspot makefiles also use -D__STDC_, so I hope both places will be fixed
> together.
>
> >
> > test/hotspot/jtreg/vmTestbase/gc/g1/unloading/libdefine.cpp
> >
> >  #ifdef __cplusplus
> > ! #define JNI_ENV_ARG(x, y) y
> >  #define JNI_ENV_PTR(x) x
> >  #else
> >  #define JNI_ENV_ARG(x, y) x , y
> >  #define JNI_ENV_PTR(x) (*x)
> >  #endif
> >
> > If this is now a C++ source file why do you a still have the code that
> allows for either C or C++? Is this stuff that will be cleaned up in the
> later RFE?
> yes, 8209547 will clean up JNI_ENV_ARG macros.
>
> >
> > ---
> >
> > The switch to C++ means a lot of code now needs:
> >
> > + #ifdef __cplusplus
> > + extern "C" {
> > + #endif
>
> that's true, and this patch adds 'extern "C"' very conservatively
> (basically to everywhere). this, if needed, can be cleaned up later.
>
> >
> > I still have to wonder whether it better to leave these as C tests and
> only convert to C++ as and when needed ... seems like so much work.
> >
> > Anyway I'm off on vacation after today so I'll leave it to others to
> take this up.
> >
> > Thanks,
> > David
> >
> >> Thanks,
> >> -- Igor
> >>> On Aug 20, 2018, at 11:07 PM, Igor Ignatyev <igor.ignatyev at oracle.com
> <mailto:igor.ignatyev at oracle.com>> wrote:
> >>>
> >>>>> It has been discussed (not widely enough and I accept that) in
> 8209547 and the related email thread b/w JC(cc'ed) and myself.
> >>>>> as I said, I might went a way too far, so I'll revert changes in the
> non-vmTestbase tests and made appropriate changes in makefiles. what do you
> think?
> >>>>
> >>>> I think I need to see what you mean exactly :)
> >>> sure, it will take some time for me to do that, hopefully will upload
> new webrevs tomorrow morning PT. but the basic idea is to leave files in
> test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability,
> testlibrary as .c files, exactly as they were before, and restore
> corresponding filenames in make/test/JtregNativeHotspot.gmk.
>
>

-- 

Thanks,
Jc