Request for Review: Conditionalize source so that functionality can be easily included or excluded

Coleen Phillimore coleen.phillimore at oracle.com
Tue Oct 9 07:12:27 PDT 2012


>>
>> On 9/10/2012 10:36 AM, Coleen Phillimore wrote:
>>>
>>> Hi Joe,
>>>
>>> Did you take out the Windows "kernel" build with this change? If so,
>>> there are still code which refers to kernel in
>>> http://cr.openjdk.java.net/~jprovino/7189254/webrev.10/make/Makefile.html 
>>> which
>>> should be removed.
>>
>> "kernel" has not been completely eradicated but it is no longer built 
>> at all. Full eradication is a later clean up. (Joe: do we have a CR 
>> for that yet?)
>
> Not that I know of.  I'll do that.

Okay, this is fine.  Please file a bug and send me the number.

Thanks,
Coleen
>
>>
>>> In make/Makefile, the COMMON_VM_*_TARGETS is what caused the kernel to
>>> be built with jprt. This was pretty useful so that people didn't
>>> inadvertently break the kernel build. I think you should add 
>>> minimal1 to
>>> this list instead now so that the same doesn't happen with minimal vm.
>>
>> The minimal VM is not intended to always be built, but will be, for 
>> example, as part of building SE Embedded, hence JPRT will be testing 
>> the minimal VM build works.
>>
>> Going forward, in the new build system which VMs get built is 
>> determined by the JVM_VARIANT_* variables, which in turn are set at 
>> configure time. So the whole notion of "common" VM targets will be 
>> going away soon. (There are default settings for these variables for 
>> non-configure based builds.)
>>
>>> In
>>> http://cr.openjdk.java.net/~jprovino/7189254/webrev.10/src/share/vm/code/nmethod.cpp.udiff.html 
>>>
>>>
>>>
>>> Do you not need the conditional compilation because
>>> JvmtiExport::has_redefined_a_class() is included and always returns 
>>> false?
>>
>> Good catch - the conditional compilation is not needed there. (The 
>> same function is unguarded elsewhere.)
>
> I'll get rid of the unnecessary conditional.
>
>>
>>> If kernel is completely removed, there's some code you can remove in
>>> systemDictionary.cpp as well. I think that's the last of the kernel 
>>> code.
>>
>> See above - kernel not completely eradicated.
>>
>> Thanks,
>> David
>> -----
>>
>>> Rest looks good.
>
> Thanks for the review!
>
> joe
>
>>> Coleen
>>>
>>> On 10/5/2012 10:59 AM, Joe Provino wrote:
>>>> The latest and hopefully final webrev is here:
>>>>
>>>> http://cr.openjdk.java.net/~jprovino/7189254/webrev.10
>>>> <http://cr.openjdk.java.net/%7Ejprovino/7189254/webrev.10/>
>>>>
>>>> The changes are to make/{linux,bsd}/gcc.make to ensure that
>>>> CFLAGS and OPT_CFLAGS behave the way they did before any changes.
>>>>
>>>> joe
>>>>


More information about the hotspot-dev mailing list