Add FINAL to base/compiler_specific.

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

[Nico, 2011-05-31 14:31:36]

cc'ng all base OWNERS, since I don't want anyone to say "huh, I don't think this
is a good idea" four weeks in :-)

For what it's worth, I plan to use this in about 5 different places.

[Evan Martin, 2011-05-31 14:34:48]

Would another solution be to add a no-op virtual dtor to classes like
your WorkerClass example?
How bad does that feel to you?  It might help reduce confusing
keywords (even if 90% of your coworkers get this right that still
means there are tens of people getting this wrong).

On Tue, May 31, 2011 at 7:31 AM,  <thakis@chromium.org> wrote:
> Reviewers: darin, brettw,
>
> Message:
> cc'ng all base OWNERS, since I don't want anyone to say "huh, I don't think
> this
> is a good idea" four weeks in :-)
>
> For what it's worth, I plan to use this in about 5 different places.
>
> Description:
> 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
> }
>
> 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
>
>
> Please review this at http://codereview.chromium.org/7094005/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M base/compiler_specific.h
>
>
> Index: base/compiler_specific.h
> diff --git a/base/compiler_specific.h b/base/compiler_specific.h
> index
>
0a6e05a6c67988b64b416b612d47ec41c457a78f..c61641219455c72db67c605feb9e8c2638727ea8
> 100644
> --- a/base/compiler_specific.h
> +++ b/base/compiler_specific.h
> @@ -137,4 +137,15 @@
>  // If available, it would look like:
>  //   __attribute__((format(wprintf, format_param, dots_param)))
>
> +// Annotate a class to indicate that it cannot be subclassed. Use this very
> +// sparingly.  The only valid use case is if you have a class with virtual
> +// methods but no virtual destructor -- see http://crbug.com/84424
> +// Use like:
> +//   class B FINAL : public A {};
> +#if defined(__clang__)
> +#define FINAL final
> +#else
> +#define FINAL
> +#endif
> +
>  #endif  // BASE_COMPILER_SPECIFIC_H_
>
>
>

[Mark Mentovai, 2011-05-31 14:41:05]

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#newcod...
base/compiler_specific.h:145: #if defined(__clang__)
Should we have COMPILER_CLANG?

http://codereview.chromium.org/7094005/diff/1/base/compiler_specific.h#newcod...
base/compiler_specific.h:146: #define FINAL final
I’m surprised it’s final and not __final__ or an __attribute__.

[willchan no longer on Chromium, 2011-05-31 14:50:49]

I'm tentatively in favor of this suggestion. My understanding is the
perf gains are small of keeping it non-virtual, and if the object is
stack allocated (why are we doing scoped_ptr<WorkerClass> c(new
WorkerClass); instead of WorkerClass c?) the compiler will know to
skip the virtual dispatch anyway.

On Tue, May 31, 2011 at 2:34 PM, Evan Martin <evan@chromium.org> wrote:
> Would another solution be to add a no-op virtual dtor to classes like
> your WorkerClass example?
> How bad does that feel to you?  It might help reduce confusing
> keywords (even if 90% of your coworkers get this right that still
> means there are tens of people getting this wrong).
>
> On Tue, May 31, 2011 at 7:31 AM,  <thakis@chromium.org> wrote:
>> Reviewers: darin, brettw,
>>
>> Message:
>> cc'ng all base OWNERS, since I don't want anyone to say "huh, I don't think
>> this
>> is a good idea" four weeks in :-)
>>
>> For what it's worth, I plan to use this in about 5 different places.
>>
>> Description:
>> 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
>> }
>>
>> 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
>>
>>
>> Please review this at http://codereview.chromium.org/7094005/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files:
>>  M base/compiler_specific.h
>>
>>
>> Index: base/compiler_specific.h
>> diff --git a/base/compiler_specific.h b/base/compiler_specific.h
>> index
>>
0a6e05a6c67988b64b416b612d47ec41c457a78f..c61641219455c72db67c605feb9e8c2638727ea8
>> 100644
>> --- a/base/compiler_specific.h
>> +++ b/base/compiler_specific.h
>> @@ -137,4 +137,15 @@
>>  // If available, it would look like:
>>  //   __attribute__((format(wprintf, format_param, dots_param)))
>>
>> +// Annotate a class to indicate that it cannot be subclassed. Use this very
>> +// sparingly.  The only valid use case is if you have a class with virtual
>> +// methods but no virtual destructor -- see http://crbug.com/84424
>> +// Use like:
>> +//   class B FINAL : public A {};
>> +#if defined(__clang__)
>> +#define FINAL final
>> +#else
>> +#define FINAL
>> +#endif
>> +
>>  #endif  // BASE_COMPILER_SPECIFIC_H_
>>
>>
>>
>

[Nico, 2011-05-31 15:00:55]

On Tue, May 31, 2011 at 7:50 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> I'm tentatively in favor of this suggestion. My understanding is the
> perf gains are small of keeping it non-virtual, and if the object is
> stack allocated (why are we doing scoped_ptr<WorkerClass> c(new
> WorkerClass); instead of WorkerClass c?) the compiler will know to
> skip the virtual dispatch anyway.


As the comment says, this is a simplified example. The most common case is
that an object is scoped_refptr'd through a NewRunnableMethod(), the other
common case is an object being deleted through DefaultSingletonTraits
registered AtExit by a singleton. You can look at
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/18...
some of the call stacks (most of the errors in that output are false
positives, but some are real errors).

I'm slightly in favor of final, because it's the "correct" way to deal with
the "interface with protected destructor" pattern.

If we're saying "just add a virtual destructor to leaf classes", we might as
well enforce that all interface objects have virtual destructors and forbid
the use of interfaces with protected destructors, right?


>
> On Tue, May 31, 2011 at 2:34 PM, Evan Martin <evan@chromium.org> wrote:
> > Would another solution be to add a no-op virtual dtor to classes like
> > your WorkerClass example?
> > How bad does that feel to you?  It might help reduce confusing
> > keywords (even if 90% of your coworkers get this right that still
> > means there are tens of people getting this wrong).
> >
> > On Tue, May 31, 2011 at 7:31 AM,  <thakis@chromium.org> wrote:
> >> Reviewers: darin, brettw,
> >>
> >> Message:
> >> cc'ng all base OWNERS, since I don't want anyone to say "huh, I don't
> think
> >> this
> >> is a good idea" four weeks in :-)
> >>
> >> For what it's worth, I plan to use this in about 5 different places.
> >>
> >> Description:
> >> 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
> >> }
> >>
> >> 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
> >>
> >>
> >> Please review this at http://codereview.chromium.org/7094005/
> >>
> >> SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >>
> >> Affected files:
> >>  M base/compiler_specific.h
> >>
> >>
> >> Index: base/compiler_specific.h
> >> diff --git a/base/compiler_specific.h b/base/compiler_specific.h
> >> index
> >>
>
0a6e05a6c67988b64b416b612d47ec41c457a78f..c61641219455c72db67c605feb9e8c2638727ea8
> >> 100644
> >> --- a/base/compiler_specific.h
> >> +++ b/base/compiler_specific.h
> >> @@ -137,4 +137,15 @@
> >>  // If available, it would look like:
> >>  //   __attribute__((format(wprintf, format_param, dots_param)))
> >>
> >> +// Annotate a class to indicate that it cannot be subclassed. Use this
> very
> >> +// sparingly.  The only valid use case is if you have a class with
> virtual
> >> +// methods but no virtual destructor -- see http://crbug.com/84424
> >> +// Use like:
> >> +//   class B FINAL : public A {};
> >> +#if defined(__clang__)
> >> +#define FINAL final
> >> +#else
> >> +#define FINAL
> >> +#endif
> >> +
> >>  #endif  // BASE_COMPILER_SPECIFIC_H_
> >>
> >>
> >>
> >
>

[willchan no longer on Chromium, 2011-05-31 15:27:48]

On Tue, May 31, 2011 at 3:00 PM, Nico Weber <thakis@chromium.org> wrote:
> On Tue, May 31, 2011 at 7:50 AM, William Chan (陈智昌) <willchan@chromium.org>
> wrote:
>>
>> I'm tentatively in favor of this suggestion. My understanding is the
>> perf gains are small of keeping it non-virtual, and if the object is
>> stack allocated (why are we doing scoped_ptr<WorkerClass> c(new
>> WorkerClass); instead of WorkerClass c?) the compiler will know to
>> skip the virtual dispatch anyway.
>
> As the comment says, this is a simplified example. The most common case is
> that an object is scoped_refptr'd through a NewRunnableMethod(), the other
> common case is an object being deleted through DefaultSingletonTraits
> registered AtExit by a singleton. You can look
>
at http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/1857/steps/compile/logs/stdio
> for some of the call stacks (most of the errors in that output are false
> positives, but some are real errors).

I don't think optimizing a virtual dispatch for Singleton objects is
very valuable.

> I'm slightly in favor of final, because it's the "correct" way to deal with
> the "interface with protected destructor" pattern.

While I agree that it's "correct", it's not immediately obvious to me
that the extra complexity is worth it. I don't know that it'll be
obvious to people that the reason a class is marked FINAL is to
prevent the lack of virtual dispatch from biting us. If they need to
subclass it, they might just remove the FINAL keyword. I guess clang
would then flag this as a bug if someone was deleting through the
pointer to the class that was previously marked as FINAL.

Maybe I'm overconcerned. I am not bothered enough by the complexity to
block this change, so if others are cool with it, then I am too.

> If we're saying "just add a virtual destructor to leaf classes", we might as
> well enforce that all interface objects have virtual destructors and forbid
> the use of interfaces with protected destructors, right?

Yes. That's what I would encourage.

>
>>
>> On Tue, May 31, 2011 at 2:34 PM, Evan Martin <evan@chromium.org> wrote:
>> > Would another solution be to add a no-op virtual dtor to classes like
>> > your WorkerClass example?
>> > How bad does that feel to you?  It might help reduce confusing
>> > keywords (even if 90% of your coworkers get this right that still
>> > means there are tens of people getting this wrong).
>> >
>> > On Tue, May 31, 2011 at 7:31 AM,  <thakis@chromium.org> wrote:
>> >> Reviewers: darin, brettw,
>> >>
>> >> Message:
>> >> cc'ng all base OWNERS, since I don't want anyone to say "huh, I don't
>> >> think
>> >> this
>> >> is a good idea" four weeks in :-)
>> >>
>> >> For what it's worth, I plan to use this in about 5 different places.
>> >>
>> >> Description:
>> >> 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
>> >> }
>> >>
>> >> 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
>> >>
>> >>
>> >> Please review this at http://codereview.chromium.org/7094005/
>> >>
>> >> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>> >>
>> >> Affected files:
>> >>  M base/compiler_specific.h
>> >>
>> >>
>> >> Index: base/compiler_specific.h
>> >> diff --git a/base/compiler_specific.h b/base/compiler_specific.h
>> >> index
>> >>
>> >>
0a6e05a6c67988b64b416b612d47ec41c457a78f..c61641219455c72db67c605feb9e8c2638727ea8
>> >> 100644
>> >> --- a/base/compiler_specific.h
>> >> +++ b/base/compiler_specific.h
>> >> @@ -137,4 +137,15 @@
>> >>  // If available, it would look like:
>> >>  //   __attribute__((format(wprintf, format_param, dots_param)))
>> >>
>> >> +// Annotate a class to indicate that it cannot be subclassed. Use this
>> >> very
>> >> +// sparingly.  The only valid use case is if you have a class with
>> >> virtual
>> >> +// methods but no virtual destructor -- see http://crbug.com/84424
>> >> +// Use like:
>> >> +//   class B FINAL : public A {};
>> >> +#if defined(__clang__)
>> >> +#define FINAL final
>> >> +#else
>> >> +#define FINAL
>> >> +#endif
>> >> +
>> >>  #endif  // BASE_COMPILER_SPECIFIC_H_
>> >>
>> >>
>> >>
>> >
>
>

[Nico, 2011-05-31 16:09:18]

On Tue, May 31, 2011 at 8:27 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Tue, May 31, 2011 at 3:00 PM, Nico Weber <thakis@chromium.org> wrote:
> > On Tue, May 31, 2011 at 7:50 AM, William Chan (陈智昌) <
> willchan@chromium.org>
> > wrote:
> >>
> >> I'm tentatively in favor of this suggestion. My understanding is the
> >> perf gains are small of keeping it non-virtual, and if the object is
> >> stack allocated (why are we doing scoped_ptr<WorkerClass> c(new
> >> WorkerClass); instead of WorkerClass c?) the compiler will know to
> >> skip the virtual dispatch anyway.
> >
> > As the comment says, this is a simplified example. The most common case
> is
> > that an object is scoped_refptr'd through a NewRunnableMethod(), the
> other
> > common case is an object being deleted through DefaultSingletonTraits
> > registered AtExit by a singleton. You can look
> > at
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/18...
> > for some of the call stacks (most of the errors in that output are false
> > positives, but some are real errors).
>
> I don't think optimizing a virtual dispatch for Singleton objects is
> very valuable.
>
> > I'm slightly in favor of final, because it's the "correct" way to deal
> with
> > the "interface with protected destructor" pattern.
>
> While I agree that it's "correct", it's not immediately obvious to me
> that the extra complexity is worth it. I don't know that it'll be
> obvious to people that the reason a class is marked FINAL is to
> prevent the lack of virtual dispatch from biting us. If they need to
> subclass it, they might just remove the FINAL keyword. I guess clang
> would then flag this as a bug if someone was deleting through the
> pointer to the class that was previously marked as FINAL.
>
> Maybe I'm overconcerned. I am not bothered enough by the complexity to
> block this change, so if others are cool with it, then I am too.
>
> > If we're saying "just add a virtual destructor to leaf classes", we might
> as
> > well enforce that all interface objects have virtual destructors and
> forbid
> > the use of interfaces with protected destructors, right?
>
> Yes. That's what I would encourage.
>

Cool, then this is my favorite as well. This has been proposed a few times
before iirc, and found no majority (due to darin and brett I think?). So
let's see what they say.


>
> >
> >>
> >> On Tue, May 31, 2011 at 2:34 PM, Evan Martin <evan@chromium.org> wrote:
> >> > Would another solution be to add a no-op virtual dtor to classes like
> >> > your WorkerClass example?
> >> > How bad does that feel to you?  It might help reduce confusing
> >> > keywords (even if 90% of your coworkers get this right that still
> >> > means there are tens of people getting this wrong).
> >> >
> >> > On Tue, May 31, 2011 at 7:31 AM,  <thakis@chromium.org> wrote:
> >> >> Reviewers: darin, brettw,
> >> >>
> >> >> Message:
> >> >> cc'ng all base OWNERS, since I don't want anyone to say "huh, I don't
> >> >> think
> >> >> this
> >> >> is a good idea" four weeks in :-)
> >> >>
> >> >> For what it's worth, I plan to use this in about 5 different places.
> >> >>
> >> >> Description:
> >> >> 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
> >> >> }
> >> >>
> >> >> 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
> >> >>
> >> >>
> >> >> Please review this at http://codereview.chromium.org/7094005/
> >> >>
> >> >> SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >> >>
> >> >> Affected files:
> >> >>  M base/compiler_specific.h
> >> >>
> >> >>
> >> >> Index: base/compiler_specific.h
> >> >> diff --git a/base/compiler_specific.h b/base/compiler_specific.h
> >> >> index
> >> >>
> >> >>
>
0a6e05a6c67988b64b416b612d47ec41c457a78f..c61641219455c72db67c605feb9e8c2638727ea8
> >> >> 100644
> >> >> --- a/base/compiler_specific.h
> >> >> +++ b/base/compiler_specific.h
> >> >> @@ -137,4 +137,15 @@
> >> >>  // If available, it would look like:
> >> >>  //   __attribute__((format(wprintf, format_param, dots_param)))
> >> >>
> >> >> +// Annotate a class to indicate that it cannot be subclassed. Use
> this
> >> >> very
> >> >> +// sparingly.  The only valid use case is if you have a class with
> >> >> virtual
> >> >> +// methods but no virtual destructor -- see http://crbug.com/84424
> >> >> +// Use like:
> >> >> +//   class B FINAL : public A {};
> >> >> +#if defined(__clang__)
> >> >> +#define FINAL final
> >> >> +#else
> >> >> +#define FINAL
> >> >> +#endif
> >> >> +
> >> >>  #endif  // BASE_COMPILER_SPECIFIC_H_
> >> >>
> >> >>
> >> >>
> >> >
> >
> >
>

[brettw, 2011-06-01 02:37:06]

On Tue, May 31, 2011 at 9:09 AM, Nico Weber <thakis@chromium.org> wrote:
> On Tue, May 31, 2011 at 8:27 AM, William Chan (陈智昌) <willchan@chromium.org>
> wrote:
>>
>> On Tue, May 31, 2011 at 3:00 PM, Nico Weber <thakis@chromium.org> wrote:
>> > On Tue, May 31, 2011 at 7:50 AM, William Chan (陈智昌)
>> > <willchan@chromium.org>
>> > wrote:
>> >>
>> >> I'm tentatively in favor of this suggestion. My understanding is the
>> >> perf gains are small of keeping it non-virtual, and if the object is
>> >> stack allocated (why are we doing scoped_ptr<WorkerClass> c(new
>> >> WorkerClass); instead of WorkerClass c?) the compiler will know to
>> >> skip the virtual dispatch anyway.
>> >
>> > As the comment says, this is a simplified example. The most common case
>> > is
>> > that an object is scoped_refptr'd through a NewRunnableMethod(), the
>> > other
>> > common case is an object being deleted through DefaultSingletonTraits
>> > registered AtExit by a singleton. You can look
>> >
>> >
at http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/1857/steps/compile/logs/stdio
>> > for some of the call stacks (most of the errors in that output are false
>> > positives, but some are real errors).
>>
>> I don't think optimizing a virtual dispatch for Singleton objects is
>> very valuable.
>>
>> > I'm slightly in favor of final, because it's the "correct" way to deal
>> > with
>> > the "interface with protected destructor" pattern.
>>
>> While I agree that it's "correct", it's not immediately obvious to me
>> that the extra complexity is worth it. I don't know that it'll be
>> obvious to people that the reason a class is marked FINAL is to
>> prevent the lack of virtual dispatch from biting us. If they need to
>> subclass it, they might just remove the FINAL keyword. I guess clang
>> would then flag this as a bug if someone was deleting through the
>> pointer to the class that was previously marked as FINAL.
>>
>> Maybe I'm overconcerned. I am not bothered enough by the complexity to
>> block this change, so if others are cool with it, then I am too.
>>
>> > If we're saying "just add a virtual destructor to leaf classes", we
>> > might as
>> > well enforce that all interface objects have virtual destructors and
>> > forbid
>> > the use of interfaces with protected destructors, right?
>>
>> Yes. That's what I would encourage.
>
> Cool, then this is my favorite as well. This has been proposed a few times
> before iirc, and found no majority (due to darin and brett I think?). So
> let's see what they say.

It doesn't look like this "final" usage is in the C++0x at all. Is my
understanding correct? I spent a while looking for it. They have a
[[final]] attribute for functions.

I don't really care that much about this specific case, but in general
I'm getting a little concerned about the increasingly complex
non-standard C++ dialect we seem to be creating. It certainly pays off
in a lot of ways, but it also adds complexity. My suspicion is that
the only person that will use this keyword will be Nico. The only time
this comes up is when you break the Clang build, and then you're just
trying to fix it as fast as possible, which means adding virtual
destructors because there' no way you can remember all of these
macros.

Brett

[Nico, 2011-06-02 14:58:52]

> It doesn't look like this "final" usage is in the C++0x at all. Is my
> understanding correct? I spent a while looking for it. They have a
> [[final]] attribute for functions.

I agree, the C++ standard is very hard to navigate. It's in 9.3 (
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf ): "If a class
is marked with the class-virt-specifier final and it appears as a
base-type-specifier in a base-clause (Clause 10), the program is ill-formed." For
what it's worth, MSVC2010 supports it too, even though they use the spelling
from an earlier standards draft – it's currently called "sealed" in MSVC. (Note
that the [[]] attribute syntax for final etc has been replaced with contextual
keywords, see
http://herbsutter.com/2010/12/08/trip-report-november-2010-c-standards-meeting/
)

> I don't really care that much about this specific case, but in general
> I'm getting a little concerned about the increasingly complex
> non-standard C++ dialect we seem to be creating.

You mean the other stuff in base/compiler_specific.h?

> It certainly pays off in a lot of ways, but it also adds complexity.

This is a fair point. So far it looks like you, Evan, and Will are all for
abandoning the "interface class with protected non-virtual destructor" pattern.
If Darin agrees to this as well (Darin?), that's fine with me.

[Nico, 2011-06-02 23:59:32]

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?

[willchan no longer on Chromium, 2011-06-03 16:56:54]

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

On Fri, Jun 3, 2011 at 1:59 AM,  <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/
>

[Nico, 2011-06-03 21:27:25]

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