RFR: 8134797: Remove explicit casts in CollectorPolicy hierarchy

[Kim Barrett]

On Sep 1, 2015, at 3:54 AM, Per Liden <per.liden at oracle.com> wrote:
> 
> Hi Kim,

Hi Per - Thanks for looking at this.

> On 2015-09-01 03:04, Kim Barrett wrote:
>> Please review this change to eliminate explicit casts within the
>> CollectorPolicy hierarchy.  All of the casts being eliminated were
>> C-style casts; there are no C++ style casts in this hierarchy.
>> 
>> For downcasts to GenCollectorPolicy* the policy was being obtained
>> from a GenCollectedHeap, so we just use gen_policy instead of an
>> unchecked downcast of collector_policy.
>> 
>> For the remaining downcast (to ConcurrentMarkSweepPolicy*) we now
>> assert the corresponding is_xxx_policy and use as_xxx_policy to
>> perform the conversion.
>> 
>> Changed the explicit upcasts to CollectorPolicy* to implict
>> conversions.  For the problematic one, moved to a scope where
>> G1CollectorPolicy is complete, so the now implicit conversion works.
>> I'm kind of surprised that one didn't cause obvious problems.
> 
> Hmm, if it was incomplete, wouldn't the compiler have complained?

Nope, the compiler will just believe you meant a reinterpret_cast, and that such
a cast is sensible (which is nearly never true for not-standard-layout classes),
else you wouldn’t have asked for it. This is one of the dangers with C-style casts.
It was in part something similar that led to much frustrated debugging for one of
our senior developers a few months ago.

If it were up to me, C-style casts would be forbidden in our code.  With more than
7K of them, I think there’s a lot of opportunities for improvement...

> When I look at the include dependencies it seems like g1CollectedHeap.o indirectly includes g1CollectorPolicy.hpp, so this shouldn't be a problem (but g1CollectorPolicy should be explicitly included).

I don’t understand?  g1CollectedHeap.cpp already has:
36 #include "gc/g1/g1CollectorPolicy.hpp”

> I have no strong opinion about moving the function to the .cpp file, but I would let if stay in the .hpp unless there's really a problem.

The alternative to moving the collector_policy definition was to add an
#include of g1CollectorPolicy.hpp to g1CollectedHeap.hpp.

Since collector_policy is a virtual function, and we don’t have C++ final specifiers,
I don’t think it is a likely candidate for inlining, so I moved it to the .cpp file.