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.hol...@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.ignat...@oracle.com >>> <mailto:igor.ignat...@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.
