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

[JC Beyler]

Sounds good to me, thanks for doing it.

Let's push the question about static_cast to future clean-up/conversations
then.

Still looks good to me ;-)
Jc

On Wed, Aug 22, 2018 at 2:15 PM Igor Ignatyev <igor.ignatyev at oracle.com>
wrote:

> Hi JC,
>
> thanks for looking at it and even paying attention to the changes;)
>
> regarding your nits. a space before comma is straightforward and must be
> done for test code to be aligned w/ regular hotspot code style,
> static_case<> is more controversial, it's not used much by hotspot. I do
> recall some discussion around it 3-4 years ago, but I can't recall the
> outcome. anyhow, I'd prefer to clean it up later, as these editorial/style
> fixes can be done more gradually, and this patch kinda blocks your work on
> 8209547.
>
> Thanks,
> -- Igor
>
>
> On Aug 22, 2018, at 11:53 AM, JC Beyler <jcbeyler at google.com> wrote:
>
> 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
>
>
>

-- 

Thanks,
Jc