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

[Joe Provino]

On 10/8/2012 11:57 PM, David Holmes wrote:
> Hi Coleen,
>
> If I may reply in Joe's stead ...

David, thanks for replying.

Comments inline...

>
> 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.

>
>> 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
>>>