RFR: 8003310: Enable -Wunused when compiling with GCC

[Coleen Phillmore]

Looks good Mikael!  I like these sorts of changes.
Coleen

On 2/1/2013 11:35 AM, Mikael Vidstedt wrote:
>
> 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
>>>>>>
>>>>
>>>
>>
>