roll clang 131935:132017

roll clang 131935:132017

clang recently added a warning that warns when invoking 'delete' on a polymorphic non-final class without a virtual destructor. This finds real bugs – see the bug referenced below for a few examples. 

However, one common pattern where it fires is this case: 

class SomeInterface { 
public: 
virtual void interfaceMethod() {} // or = 0; 
protected: 
~SomeInterface() {} 
} 

class WorkerClass : public SomeInterface { 
public: 
// many non-virtual functions, but also: 
virtual void interfaceMethod() override { /* do actual work */ } 
}; 

void f() { 
scoped_ptr<WorkerClass> c(new WorkerClass); // simplified example 
} 

(See the 2nd half of http://www.gotw.ca/publications/mill18.htm for an explanation of this pattern.)

It is arguably correct to fire the warning here, since someone might make a subclass of WorkerClass and replace |new WorkerClass| with |new WorkerClassSubclass|. This would be broken since WorkerClass doesn't have a virtual destructor. 

The solution that the clang folks recommend is to mark WorkerClass as |final| (a c++0x keyword that clang supports as an extension in normal c++ mode – like override). But chrome's base/OWNERS deemed that as too complicated and we decided to make virtual the destructors of leaf classes that implement these interfaces and that are deleted dynamically. All of the changes in this CL are to shut up the warning, not because of real problems (I fixed these in separate CLs).

(For the gtk files, this is necessary because the CHROMEGTK_CALLBACK_ macros add virtual functions.)

BUG=84424
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88270

[Nico, 2011-06-07 22:01:59]

I think this might finally be complete.

The red linux bot is
https://bugs.launchpad.net/ubuntu/+source/binutils/+bug/680584. I need to figure
that out too, but it's unrelated to this roll – it's already happening on some
of the slaves with the current clang.

[Evan Martin, 2011-06-07 22:07:20]

Stopping early to verify I understand.

http://codereview.chromium.org/7087028/diff/19002/chrome/browser/ui/gtk/crypt...
File chrome/browser/ui/gtk/crypto_module_password_dialog.cc (right):

http://codereview.chromium.org/7087028/diff/19002/chrome/browser/ui/gtk/crypt...
chrome/browser/ui/gtk/crypto_module_password_dialog.cc:89: virtual
~CryptoModulePasswordDialog() {}
Why is this one needed?  It seems like this class has no superclass or virtual
methods...

[Evan Martin, 2011-06-07 22:07:38]

On 2011/06/07 22:07:20, Evan Martin wrote:
> Stopping early to verify I understand.
> 
>
http://codereview.chromium.org/7087028/diff/19002/chrome/browser/ui/gtk/crypt...
> File chrome/browser/ui/gtk/crypto_module_password_dialog.cc (right):
> 
>
http://codereview.chromium.org/7087028/diff/19002/chrome/browser/ui/gtk/crypt...
> chrome/browser/ui/gtk/crypto_module_password_dialog.cc:89: virtual
> ~CryptoModulePasswordDialog() {}
> Why is this one needed?  It seems like this class has no superclass or virtual
> methods...

Durr, missed your comment about macros.  Disregard, I will review now.

[Evan Martin, 2011-06-07 22:09:49]

LGTM

[Nico, 2011-06-07 22:09:53]

http://codereview.chromium.org/7087028/diff/19002/chrome/browser/ui/gtk/crypt...
File chrome/browser/ui/gtk/crypto_module_password_dialog.cc (right):

http://codereview.chromium.org/7087028/diff/19002/chrome/browser/ui/gtk/crypt...
chrome/browser/ui/gtk/crypto_module_password_dialog.cc:89: virtual
~CryptoModulePasswordDialog() {}
On 2011/06/07 22:07:20, Evan Martin wrote:
> Why is this one needed?  It seems like this class has no superclass or virtual
> methods...

"(For the gtk files, this is necessary because the CHROMEGTK_CALLBACK_ macros
add virtual functions.) "

[Nico, 2011-06-08 02:18:26]

Given that this is a fairly mindless CL with LG from a base OWNER, i won't hunt
down owners for all the cleanup stuff.

(If someone has a problem with this, we need toplevel cleanup owners.)