RFR: 8003310: Enable -Wunused when compiling with GCC (original) (raw)

Coleen Phillimore coleen.phillimore at oracle.com
Tue Nov 13 06:20:16 PST 2012

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 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.

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::testglobals() { + intptrt pagesize = 4096; Page size may not be 4K - will the test still be valid? The comments describing clampaddressinpage 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 (samepage) which was duplicated in four different places. I merged it to a single function, renamed it to clampaddressinpage, 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

More information about the hotspot-dev mailing list