Check that classes with non-trivial destructors have finalization support.

Check that classes with non-trivial destructors have finalization support.

Uses clang's notion of when a class has a 'trivial destructor' to determine when garbage-collected types need finalization.

BUG=334149
NOTRY=true

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

[zerny-chromium, 2014-03-12 16:22:49]

Ready for review. PTAL.

[zerny-chromium, 2014-03-12 16:23:28]

This is the plugin side change of CL: https://codereview.chromium.org/189543014/

[Vyacheslav Egorov (Chromium), 2014-03-12 18:24:24]

lgtm

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

https://codereview.chromium.org/192933002/diff/40001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp:45: "[blink-gc] Non-finalized
class %0 requires finalization.";
I would remove "Non-finalized" it is confusing. 

Class %0 requires finalization. It must inherit from GarbageCollectedFinalized.

https://codereview.chromium.org/192933002/diff/40001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/class_requires_finalization_field.h
(right):

https://codereview.chromium.org/192933002/diff/40001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/class_requires_finalization_field.h:53:
HeapVector<Member<A>, 10> m_as;
What about 

class C {
public:
  void trace(Visitor*);
};

class AlsoDoesNotNeedFinalizer : public A {
public:
  HeapVector<C, 10> m_as;
};

it's a mix of A and B cases.

[zerny-chromium, 2014-03-13 09:32:21]

Thanks for the review Slava.

Nico: would you like me to hold out on pushing plugin updates until clang has
rolled?

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

https://codereview.chromium.org/192933002/diff/40001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp:45: "[blink-gc] Non-finalized
class %0 requires finalization.";
On 2014/03/12 18:24:24, Vyacheslav Egorov wrote:
> I would remove "Non-finalized" it is confusing. 
> 
> Class %0 requires finalization. It must inherit from
GarbageCollectedFinalized.

Removed the "non-finalized" part. I've not added the hint because the fix might
be to removed the finalization issues or to use RefCountedGarbageCollected etc.

https://codereview.chromium.org/192933002/diff/40001/tools/clang/blink_gc_plu...
File tools/clang/blink_gc_plugin/tests/class_requires_finalization_field.h
(right):

https://codereview.chromium.org/192933002/diff/40001/tools/clang/blink_gc_plu...
tools/clang/blink_gc_plugin/tests/class_requires_finalization_field.h:53:
HeapVector<Member<A>, 10> m_as;
On 2014/03/12 18:24:24, Vyacheslav Egorov wrote:
> What about 
> 
> class C {
> public:
>   void trace(Visitor*);
> };
> 
> class AlsoDoesNotNeedFinalizer : public A {
> public:
>   HeapVector<C, 10> m_as;
> };
> 
> it's a mix of A and B cases.

Done.

[Nico, 2014-03-13 16:11:55]

On Thu, Mar 13, 2014 at 2:32 AM, <zerny@chromium.org> wrote:

> Thanks for the review Slava.
>
> Nico: would you like me to hold out on pushing plugin updates until clang
> has
> rolled?
>

As long as your code compiles and you're not surprised by it suddenly
getting deployed, landing plugin changes shouldn't interfere with clang
rolls.


>
>
> https://codereview.chromium.org/192933002/diff/40001/
> tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp
> File tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp (right):
>
> https://codereview.chromium.org/192933002/diff/40001/
> tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp#newcode45
> tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp:45: "[blink-gc]
> Non-finalized class %0 requires finalization.";
> On 2014/03/12 18:24:24, Vyacheslav Egorov wrote:
>
>> I would remove "Non-finalized" it is confusing.
>>
>
>  Class %0 requires finalization. It must inherit from
>>
> GarbageCollectedFinalized.
>
> Removed the "non-finalized" part. I've not added the hint because the
> fix might be to removed the finalization issues or to use
> RefCountedGarbageCollected etc.
>
> https://codereview.chromium.org/192933002/diff/40001/
> tools/clang/blink_gc_plugin/tests/class_requires_finalization_field.h
> File
> tools/clang/blink_gc_plugin/tests/class_requires_finalization_field.h
> (right):
>
> https://codereview.chromium.org/192933002/diff/40001/
> tools/clang/blink_gc_plugin/tests/class_requires_
> finalization_field.h#newcode53
> tools/clang/blink_gc_plugin/tests/class_requires_finalization_field.h:53:
> HeapVector<Member<A>, 10> m_as;
> On 2014/03/12 18:24:24, Vyacheslav Egorov wrote:
>
>> What about
>>
>
>  class C {
>> public:
>>    void trace(Visitor*);
>> };
>>
>
>  class AlsoDoesNotNeedFinalizer : public A {
>> public:
>>    HeapVector<C, 10> m_as;
>> };
>>
>
>  it's a mix of A and B cases.
>>
>
> Done.
>
> https://codereview.chromium.org/192933002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[zerny-chromium, 2014-03-14 06:15:29]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-14 06:16:53]

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

[commit-bot: I haz the power, 2014-03-14 07:39:49]

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

[commit-bot: I haz the power, 2014-03-14 07:39:50]

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

[zerny-chromium, 2014-03-14 10:44:10]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-14 10:44:19]

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

[commit-bot: I haz the power, 2014-03-14 10:45:16]

Change committed as 257072