RFR: 8003310: Enable -Wunused when compiling with GCC

[Mikael Vidstedt]

On 2012-11-13 06:20, Coleen Phillimore wrote:
> On 11/13/2012 8:41 AM, Coleen Phillimore wrote:
>>
>> Hi Mikael,   This is good!  I have the same comments as David.
>>
>> For constantPool.cpp can you add #undef DBG after that code.
>>
>> Maybe there should be a new convention for DEBUG_<filename in caps> 
>> for unused code that people want to leave around for debugging.   I 
>> don't recommend we add a lot of code like this, but having such an 
>> ifdef would differentiate code that was just left in unintentionally 
>> from code that we want to be left in (occasionally).
>
> Actually, I want to revise this.  We had this discussion about this 
> recently rwt metaspace debugging and favored a const static variable = 
> false; under #ifdef DEBUG or ASSERT and the debugging code under 
> DEBUG/ASSERT so that it doesn't bit rot.   This is for the code left 
> in intentionally.   Which makes me wonder why print_cpool_bytes() is 
> there under DEBUG_CPOOL claiming to be "temporary" when there is 
> another printing function in that file already.

The reason why I moved it under DBG was because it is only ever called 
wrapped with a DBG(). I can play around with making it a const static = 
false as you outlined.

The other print functions in constantPool.cpp are implementations of the 
virtual functions inherited from Metadata, so they are not quite as 
temporary as print_cpool_bytes, if that makes sense.

Cheers,
Mikael

>
> Coleen
>
>>
>> Thanks,
>> Coleen
>>
>> On 11/13/2012 12:47 AM, David Holmes wrote:
>>> Hi Mikael,
>>>
>>> A couple of general observations as really the "owners" of each file 
>>> needs to assess the changes:
>>>
>>> - sometimes functions exist for debugging/tracing and calls will be 
>>> added to the code by engineers as they debug. For example MBFence in 
>>> synchronizer.cpp allows you to add fences into expressions.
>>>
>>> - why was the "static" removed from a number functions. They now 
>>> have global visibility rather than being restricted to their files?
>>>
>>>
>>> In globaleDefinitions.cpp:
>>>
>>> + void GlobalDefinitions::test_globals() {
>>> +   intptr_t page_size = 4096;
>>>
>>> Page size may not be 4K - will the test still be valid?
>>>
>>>
>>> The comments describing clamp_address_in_page don't need to be on 
>>> both the declaration and definition.
>>>
>>> Cheers,
>>> David
>>> ------
>>>
>>> On 13/11/2012 1:59 PM, Mikael Vidstedt wrote:
>>>>
>>>> All,
>>>>
>>>> Please review the below change. The change adds the -Wunused flag when
>>>> compiling with GCC and removes a number of unused functions/dead code.
>>>>
>>>> In the process I found one function (same_page) which was 
>>>> duplicated in
>>>> four different places. I merged it to a single function, renamed it to
>>>> clamp_address_in_page, added some comments and refactored it to be
>>>> slightly easier to understand. I also added unit tests for it. 
>>>> Feedback
>>>> appreciated (especially on the name).
>>>>
>>>> http://cr.openjdk.java.net/~mikael/8003310/webrev.00/
>>>>
>>>> Passes JPRT and the built-in unit tests (-XX:+ExecuteInternalVMTests).
>>>>
>>>> Thanks,
>>>> Mikael
>>>>
>>
>