RFR: 8003310: Enable -Wunused when compiling with GCC

[Mikael Vidstedt]

All,

I finally found some time to update the webrev:

http://cr.openjdk.java.net/~mikael/8003310/webrev.03/webrev/

Not much has changes since the first webrev. I played around with 
turning the #ifdef in constantPool.cpp into a const bool = false, but 
one of the downsides I noticed with that is that the code will still be 
compiled into product and especially for this type of function with a 
relatively large number of constant strings in it it unnecessarily adds 
to the footprint. This does not invalidate the const bool = false idiom, 
but there may be cases where it is beneficial to use alternatives to it.

Cheers,
Mikael

On 11/15/2012 5:00 PM, Mikael Vidstedt wrote:
> 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
>>>>>
>>>
>>
>