Check consistency of manual trace and finalization dispatching.

Check consistency of manual trace and finalization dispatching.

This adds checks that overridden trace methods are always virtual and that manual implementations are always on non-virtual/polymorphic classes.
At each subclass definition of a class implementing manual trace/finalize dispatching, the plugin checks for a dispatch to the subclass if possible.

BUG=334149
R=ager@chromium.org
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257710

[zerny-chromium, 2014-03-17 16:00:48]

The dispatch checks are not accurate since some subclasses are not actually
dispatched to because they act as shared code intermediates. For example:

In StyleRuleBase:
 - StyleRuleGroup

In CSSValue:
 - CSSImageGeneratorValue
 - CSSGradientValue

A few options to fixing this:
 1. Make the shared intermediate pure virtual (and ignore dispatching to
abstract classes)
 2. Insert a noop ignore/dispatch to the shared intermediate in the manual trace
dispatch method.

Any thoughts?

[zerny-chromium, 2014-03-18 07:45:59]

This patch adds checks for manual finalize dispatching. The issue regarding
dispatching to shared intermediates is solved by excluding dispatch checks for
classes that have no public constructors and no create methods.

PTAL

[Mads Ager (chromium), 2014-03-18 09:19:31]

LGTM with minor comments.

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp (right):

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp:739: if (base->IsUnmixedGCMixin())
Can we find a better name for IsUnmixedGCMixin? I don't understand what that
means.

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/RecordInfo.cpp (right):

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:302: return;
How about removing this and instead moving the below for loop into the else
branch above?

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:303: // If this class does not define
a trace dispatch method search inherit it.
search the inheritance hierarchy for it?

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/finalize_after_dispatch.cpp (right):

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/finalize_after_dispatch.cpp:14: case TB:
toB(this)->traceAfterDispatch(visitor);
I think you want some breaks here so that you do not fall through?

Could you format these switch statements in the more usual way:

case TC:
    abc();
    break;
case TD:
    def();
    break;

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/finalize_after_dispatch.cpp:27: case TB:
toB(this)->~B();
break!

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/trace_after_dispatch.cpp (right):

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/trace_after_dispatch.cpp:14: case TB:
toB(this)->traceAfterDispatch(visitor);
break!

[zerny-chromium, 2014-03-18 10:03:41]

Thanks for the review!

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp (right):

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp:739: if (base->IsUnmixedGCMixin())
On 2014/03/18 09:19:32, Mads Ager (chromium) wrote:
> Can we find a better name for IsUnmixedGCMixin? I don't understand what that
> means.

Changed to just IsGCMixin().

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/RecordInfo.cpp (right):

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:302: return;
On 2014/03/18 09:19:32, Mads Ager (chromium) wrote:
> How about removing this and instead moving the below for loop into the else
> branch above?

The for loop might need to run in either branch since trace_dispatch_method_ can
be 0 in both cases (trace might be 0).

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/RecordInfo.cpp:303: // If this class does not define
a trace dispatch method search inherit it.
On 2014/03/18 09:19:32, Mads Ager (chromium) wrote:
> search the inheritance hierarchy for it?

Yes. Thanks.

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/finalize_after_dispatch.cpp (right):

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/finalize_after_dispatch.cpp:14: case TB:
toB(this)->traceAfterDispatch(visitor);
On 2014/03/18 09:19:32, Mads Ager (chromium) wrote:
> I think you want some breaks here so that you do not fall through?
> 
> Could you format these switch statements in the more usual way:
> 
> case TC:
>     abc();
>     break;
> case TD:
>     def();
>     break;

Yes. I should avoid writing bad/invalid code in test even though they are never
run.

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/finalize_after_dispatch.cpp:27: case TB:
toB(this)->~B();
On 2014/03/18 09:19:32, Mads Ager (chromium) wrote:
> break!

Done.

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/trace_after_dispatch.cpp (right):

https://codereview.chromium.org/197863003/diff/20001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/trace_after_dispatch.cpp:14: case TB:
toB(this)->traceAfterDispatch(visitor);
On 2014/03/18 09:19:32, Mads Ager (chromium) wrote:
> break!

Done.

[zerny-chromium, 2014-03-18 10:03:50]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-18 10:04:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/197863003/60001

[commit-bot: I haz the power, 2014-03-18 11:22:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 11:22:20]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[zerny-chromium, 2014-03-18 18:36:00]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-18 18:50:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/197863003/60001

[commit-bot: I haz the power, 2014-03-18 19:09:56]

Change committed as 257710