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

Mikael Vidstedt mikael.vidstedt at oracle.com
Thu Nov 15 17:00:00 PST 2012

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 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 printcpoolbytes() is there under DEBUGCPOOL 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::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