RFR: 8003310: Enable -Wunused when compiling with GCC

[Coleen Phillimore]

No final comments.   It looks good to me.
Coleen

On 2/21/2013 3:00 PM, Mikael Vidstedt wrote:
>
> On 2013-02-20 10:04, Coleen Phillimore wrote:
>> On 2/20/2013 12:33 PM, Mikael Vidstedt wrote:
>>>
>>> On 2/19/2013 12:01 PM, Christian Thalinger wrote:
>>>> On Feb 18, 2013, at 3:14 PM, Mikael Vidstedt 
>>>> <mikael.vidstedt at oracle.com> wrote:
>>>>
>>>>> A really good control question, thanks a lot for asking!
>>>>>
>>>>> As a matter of fact these changes are basically just doing what 
>>>>> the compiler already does for us, so it's more about cleaning up 
>>>>> the source code than reducing the size of the binary. So in theory 
>>>>> these changes should have no impact on the binary size at all, but 
>>>>> it actually turns out they do. Very specifically, the fact that I 
>>>>> moved the same_page() function from being duplicated in the three 
>>>>> os_<os>.cpp files to having it be in globalDefinitions.cpp makes 
>>>>> the binary grow a few bytes (54 bytes to be specific). The reason 
>>>>> is a bit subtle:
>>>>>
>>>>> The same_page() function is (was) static in the respective 
>>>>> os_*.cpp files. They are only ever used when the Verbose flag is 
>>>>> true, and furthermore the Verbose option is a develop only flag, 
>>>>> which means it is hardcoded to false in product. The compiler 
>>>>> knows that's the case and eliminates the same_page() function 
>>>>> completely.
>>>>>
>>>>> Since I moved it to globalDefinitions.cpp there's no way easy for 
>>>>> the compiler to know that it is not being used, so it will 
>>>>> actually keep the function.
>>>>>
>>>>> Unless there are strong opinions I'm not going to do anything 
>>>>> about this.
>>>>>
>>>>> However, this made me question my earlier experiments with using 
>>>>> the const bool = false construct in constantPool.cpp, because 
>>>>> after all that is the exact same pattern. And it turns out that I 
>>>>> must have done something wrong when I was performing the 
>>>>> experiments, because when I do the same thing again now it turns 
>>>>> out the compiler actually *does* deadcode eliminate the debug-only 
>>>>> functions. So I take everything back and conclude that const bool 
>>>>> = false is indeed a great way to make sure the debug code does not 
>>>>> rot over time, and that the product binary will *not* contain the 
>>>>> dead functions. Sorry for any confusion this may have caused.
>>>>>
>>>>> With all that in mind, here's another version of the webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev04/webrev/
>>>> src/share/vm/interpreter/interpreterRuntime.cpp:
>>>>
>>>> I suppose that #if 0 is to keep that code for...?  Could we add a 
>>>> comment why we keep it?
>>>
>>> This was by "popular request" (from David Holmes) :)
>>>
>>> I personally don't know if and how this is being used. Note that 
>>> ObjectSynchronizer::trace_locking is PRODUCT_RETURN and the 
>>> non-product implementation in synchronizer.cpp is empty (modulo the 
>>> comment saying "Don't know what to do here"), so one can question 
>>> the value of keeping the any of the trace_locking functions, but 
>>> they may be someone's favorite debugging tool. Until proven 
>>> differently I will leave it in there for now. If somebody can help 
>>> me provide a useful comment about how the code is actually being 
>>> used then I will certainly add it, otherwise I'll keep this as it is.
>>
>> If I remember correctly, this trace_locking code was associated with 
>> a flag that we removed a long time ago.   I don't think if we want to 
>> implement TraceLocking we'd use this function.  My vote is that it 
>> and the ObjectMonitor:: version should be removed.
>
> Thanks Coleen, that makes sense. I've prepared a final version of the 
> webrev with the trace_locking methods removed too:
>
> http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev.06/webrev/
>
> Any final comments?
>
> Cheers,
> Mikael
>
>>
>> Coleen
>>>
>>>> src/share/vm/oops/constantPool.cpp:
>>>>
>>>> So we use:
>>>>
>>>> + const bool debug_cpool = false;
>>>>
>>>> but we still have the DBG macro.  Can't we have a static debug 
>>>> method that takes all printf arguments and prints them?  The 
>>>> debug_cpool should make that method empty and the compiler can 
>>>> remove it.  I really start to hate all these macros...
>>>
>>> I share your macro hate and I actually played around with removing 
>>> the macros just as you say. There is one small problem with doing it 
>>> that way, and that's the fact that a few of the printf:s print 
>>> symbols names, and as part of that they need to create utf8 strings 
>>> using sym->as_utf8(). The compiler can unfortunately not know that 
>>> that function is for all intents and purposes a no-op, so it will 
>>> keep the call even in product meaning increased binary size and it 
>>> will also add to the runtime since it actually performs the call and 
>>> creates the utf8 string. So I'm going to keep the macros for now.
>>>
>>>
>>> I also moved the clamp_address_in_page to the globalDefinitions.hpp 
>>> header file, meaning it will be inlined and dead code eliminated.
>>>
>>> New webrev here:
>>>
>>> http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev.05/webrev/
>>>
>>> Final comments?
>>>
>>> Cheers,
>>> Mikael
>>>
>>
>