RFR: 8134797: Remove explicit casts in CollectorPolicy hierarchy

[Per Liden]

Hi Kim,

On 2015-09-03 05:21, Kim Barrett wrote:
> 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

Ah, that's bad.

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

Agree.

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

Sorry, yes, that's what I mean, but I looked at the wrong dependency chain.

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

I'm fine with either approach. Inlining isn't a concern here.

cheers,
/Per