Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 7094005: Add FINAL to base/compiler_specific. (Closed)

Created:
8 years, 5 months ago by Nico
Modified:
8 years, 5 months ago
CC:
chromium-reviews, brettw-cc_chromium.org, Evan Martin, willchan no longer on Chromium
Visibility:
Public.

Description

Closed without committing: The decision is to make virtual the destructors of leaf classes implementing interfaces with non-virtual protected destructors instead. See the discussion in the review for details. ------ Add FINAL to base/compiler_specific. 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). Since the -Wdelete-non-virtual-dtor finds real bugs, and since the fix is arguably the Right Thing to do anyway, I'd like to follow this advise. Hence, this CL adds FINAL as an alias for |final| for clang and as a no-op for other compilers. BUG=84424 TEST=none

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -0 lines) Patch
M base/compiler_specific.h View 1 chunk +11 lines, -0 lines 2 comments Download

Messages

Total messages: 12 (0 generated)
Nico
cc'ng all base OWNERS, since I don't want anyone to say "huh, I don't think ...
8 years, 5 months ago (2011-05-31 14:31:36 UTC) #1
Evan Martin
Would another solution be to add a no-op virtual dtor to classes like your WorkerClass ...
8 years, 5 months ago (2011-05-31 14:34:48 UTC) #2
Mark Mentovai
http://codereview.chromium.org/7094005/diff/1/base/compiler_specific.h File base/compiler_specific.h (right): http://codereview.chromium.org/7094005/diff/1/base/compiler_specific.h#newcode145 base/compiler_specific.h:145: #if defined(__clang__) Should we have COMPILER_CLANG? http://codereview.chromium.org/7094005/diff/1/base/compiler_specific.h#newcode146 base/compiler_specific.h:146: #define ...
8 years, 5 months ago (2011-05-31 14:41:05 UTC) #3
willchan no longer on Chromium
I'm tentatively in favor of this suggestion. My understanding is the perf gains are small ...
8 years, 5 months ago (2011-05-31 14:50:49 UTC) #4
Nico
On Tue, May 31, 2011 at 7:50 AM, William Chan (陈智昌) <willchan@chromium.org>wrote: > I'm tentatively ...
8 years, 5 months ago (2011-05-31 15:00:55 UTC) #5
willchan no longer on Chromium
On Tue, May 31, 2011 at 3:00 PM, Nico Weber <thakis@chromium.org> wrote: > On Tue, ...
8 years, 5 months ago (2011-05-31 15:27:48 UTC) #6
Nico
On Tue, May 31, 2011 at 8:27 AM, William Chan (陈智昌) <willchan@chromium.org>wrote: > On Tue, ...
8 years, 5 months ago (2011-05-31 16:09:18 UTC) #7
brettw
On Tue, May 31, 2011 at 9:09 AM, Nico Weber <thakis@chromium.org> wrote: > On Tue, ...
8 years, 5 months ago (2011-06-01 02:37:06 UTC) #8
Nico
> It doesn't look like this "final" usage is in the C++0x at all. Is ...
8 years, 5 months ago (2011-06-02 14:58:52 UTC) #9
Nico
Talked to Darin; he's fine either way. Evan suggested to put the virtual dtors only ...
8 years, 5 months ago (2011-06-02 23:59:32 UTC) #10
willchan no longer on Chromium
I was confused until I noticed the "when possible" part. This description is misleading since ...
8 years, 5 months ago (2011-06-03 16:56:54 UTC) #11
Nico
8 years, 5 months ago (2011-06-03 21:27:25 UTC) #12
On 2011/06/03 16:56:54, willchan wrote:
> I was confused until I noticed the "when possible" part. This
> description is misleading since it doesn't mention that you need
> virtual dtors in interfaces if you delete through them. We already
> catch that though with clang, right?

We don't yet, but we will once I rolled to the clang revision that adds
-Wdelete-non-virtual-dtor (http://crbug.com/84424)

> Just make sure that when you
> announce this and update any docs that you include the full set of
> rules wrt virtual dtors. I have no objections to closing this CL.

Closed.

> 
> On Fri, Jun 3, 2011 at 1:59 AM,  <mailto:thakis@chromium.org> wrote:
> > Talked to Darin; he's fine either way.
> >
> > Evan suggested to put the virtual dtors only in leaf classes to keep
> > interface
> > definitions shorter (when possible). This rule can be simplified to 'if you
> > have
> > any virtuals and aren't an interface, you need a virtual dtor'.
> >
> > So, let's do that and close this CL. Any objections?
> >
> > http://codereview.chromium.org/7094005/
> >

Powered by Google App Engine
This is Rietveld 408576698