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 <jcbey...@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.ignat...@oracle.com > <mailto:igor.ignat...@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.hol...@oracle.com > > <mailto: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/~iignatyev//8209611/webrev.02/index.html> > >> <http://cr.openjdk.java.net/%7Eiignatyev//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> <mailto: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. > > > > -- > > Thanks, > Jc
