RFR: 8003310: Enable -Wunused when compiling with GCC

[Azeem Jiva]

Did you measure how much the libjvm.so shrank with your changes?

On 2/14/2013 7:27 PM, Coleen Phillmore wrote:
>
> 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
>>>>>>>
>>>>>
>>>>
>>>
>>
>