Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10417002:
RefCounted types should not have public destructors, net/ edition (Closed)
|
Created:
8 years, 7 months ago by Ryan Sleevi Modified:
8 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, jochen+watch-content_chromium.org, jam, joi+watch-content_chromium.org, eroman, darin-cc_chromium.org, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefCounted types should not have public destructors, net/ edition
BUG=123295
TEST=existing
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139272
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased to r139261 #
Messages
Total messages: 16 (0 generated)
willchan: for everything but content/public joi: for content/public
LGTM for content/public
Wait, I just started going through this. These types aren't refcounted...what's the change for? http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h#new... chrome/browser/io_thread.h:188: scoped_ptr<LoggingNetworkChangeObserver> network_change_observer_; I don't mind, but was this change necessary? Do we need a pointer to the subtype? http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifi... File net/base/network_change_notifier.h (right): http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifi... net/base/network_change_notifier.h:64: virtual ~OnlineStateObserver() {} These types aren't refcounted, why are their destructors being made protected?
Will is right, URLFetcherDelegate (in content/public) doesn't seem to be refcounted either. Cheers, Jói On Mon, May 21, 2012 at 2:31 PM, <willchan@chromium.org> wrote: > Wait, I just started going through this. These types aren't > refcounted...what's > the change for? > > > http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h > File chrome/browser/io_thread.h (right): > > http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h#new... > chrome/browser/io_thread.h:188: scoped_ptr<LoggingNetworkChangeObserver> > network_change_observer_; > I don't mind, but was this change necessary? Do we need a pointer to the > subtype? > > http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifi... > File net/base/network_change_notifier.h (right): > > http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifi... > net/base/network_change_notifier.h:64: virtual ~OnlineStateObserver() {} > These types aren't refcounted, why are their destructors being made > protected? > > http://codereview.chromium.org/10417002/
Sorry, I should have been more explicit. These are all /base/ classes for multiply-inheriting RefCounted classes. Unless we explicitly need ownership of the Delegate, keep the destructors protected so that their lifetimes are ambiguous at this layer - and to prevent anyone from doing scoped_ptr<Delegate> foo and then sticking the ref counted implementation inside. Make sense? On May 21, 2012 7:31 AM, <willchan@chromium.org> wrote: > Wait, I just started going through this. These types aren't > refcounted...what's > the change for? > > > http://codereview.chromium.**org/10417002/diff/1/chrome/** > browser/io_thread.h<http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h> > File chrome/browser/io_thread.h (right): > > http://codereview.chromium.**org/10417002/diff/1/chrome/** > browser/io_thread.h#newcode188<http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h#newcode188> > chrome/browser/io_thread.h:**188: scoped_ptr<** > LoggingNetworkChangeObserver> > network_change_observer_; > I don't mind, but was this change necessary? Do we need a pointer to the > subtype? > > http://codereview.chromium.**org/10417002/diff/1/net/base/** > network_change_notifier.h<http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifier.h> > File net/base/network_change_**notifier.h (right): > > http://codereview.chromium.**org/10417002/diff/1/net/base/** > network_change_notifier.h#**newcode64<http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifier.h#newcode64> > net/base/network_change_**notifier.h:64: virtual ~OnlineStateObserver() {} > These types aren't refcounted, why are their destructors being made > protected? > > http://codereview.chromium.**org/10417002/<http://codereview.chromium.org/104... >
Do you think this is a good thing to do in general? Or only when you multiply inherit? I'd tentatively lean towards saying that this is not a good thing to do in general, and the problem we run into with multiple inheritance is a code smell that indicates we're doing something wrong. Probably because multiple inheritance is stupid :) Can we solve this by getting rid of the multiple inheritance instead? How about a refcounted class that wraps these base classes you're changing? I don't like losing the polymorphic destruction, as that makes dependency injection harder/uglier. Related to this discussion here is http://www.gotw.ca/publications/mill18.htm where guideline 4 says: "A base class destructor should be either public and virtual, or protected and nonvirtual." On Mon, May 21, 2012 at 8:19 AM, Ryan Sleevi <rsleevi@chromium.org> wrote: > > Sorry, I should have been more explicit. These are all /base/ classes for multiply-inheriting RefCounted classes. > > Unless we explicitly need ownership of the Delegate, keep the destructors protected so that their lifetimes are ambiguous at this layer - and to prevent anyone from doing scoped_ptr<Delegate> foo and then sticking the ref counted implementation inside. > > Make sense? > > On May 21, 2012 7:31 AM, <willchan@chromium.org> wrote: >> >> Wait, I just started going through this. These types aren't refcounted...what's >> the change for? >> >> >> http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h >> File chrome/browser/io_thread.h (right): >> >> http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h#new... >> chrome/browser/io_thread.h:188: scoped_ptr<LoggingNetworkChangeObserver> >> network_change_observer_; >> I don't mind, but was this change necessary? Do we need a pointer to the >> subtype? >> >> http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifi... >> File net/base/network_change_notifier.h (right): >> >> http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifi... >> net/base/network_change_notifier.h:64: virtual ~OnlineStateObserver() {} >> These types aren't refcounted, why are their destructors being made >> protected? >> >> http://codereview.chromium.org/10417002/
On 2012/05/21 17:54:06, willchan wrote: > Do you think this is a good thing to do in general? Or only when you > multiply inherit? I'd tentatively lean towards saying that this is not a > good thing to do in general, and the problem we run into with multiple > inheritance is a code smell that indicates we're doing something wrong. > Probably because multiple inheritance is stupid :) We use multiple inheritance heavily, but for what are intended to be pure virtual interface classes. While there should only be one 'concrete' base class, by the time you make it to the content/ and chrome/ layers (or even the media/ or sync/ layers), M-I abounds. > > Can we solve this by getting rid of the multiple inheritance instead? How > about a refcounted class that wraps these base classes you're changing? I > don't like losing the polymorphic destruction, as that makes dependency > injection harder/uglier. For situations where the lifetime is ambiguous, or where polymorphic destruction is actually a desired trait, this is exactly what I've been recommending. In most cases, however, the polymorphic destruction is not intended - the lifetime contracts are such that "Delegates supplied to this [class/function] must outlive this [class/function]", and so making the destructor protected simply guarantees that. > > Related to this discussion here is > http://www.gotw.ca/publications/mill18.htm where guideline 4 says: "A base > class destructor should be either public and virtual, or protected and > nonvirtual." Yes, and that's certainly my thought on it, but our style guide & clang checker warn if you have virtual methods on a class without a virtual destructor. If we really wanted to be explicit, yes, such pure virtual interfaces should have concrete, non-virtual, protected destructors, since there never should be a polymorphic delete. I don't think it's been worth the (possible, speculative) compile time hit to make the existing style checker inspect the class to see if it's all pure virtual methods. > > On Mon, May 21, 2012 at 8:19 AM, Ryan Sleevi <mailto:rsleevi@chromium.org> wrote: > > > > Sorry, I should have been more explicit. These are all /base/ classes for > multiply-inheriting RefCounted classes. > > > > Unless we explicitly need ownership of the Delegate, keep the destructors > protected so that their lifetimes are ambiguous at this layer - and to > prevent anyone from doing scoped_ptr<Delegate> foo and then sticking the > ref counted implementation inside. > > > > Make sense? > > > > On May 21, 2012 7:31 AM, <mailto:willchan@chromium.org> wrote: > >> > >> Wait, I just started going through this. These types aren't > refcounted...what's > >> the change for? > >> > >> > >> http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h > >> File chrome/browser/io_thread.h (right): > >> > >> > http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h#new... > >> chrome/browser/io_thread.h:188: scoped_ptr<LoggingNetworkChangeObserver> > >> network_change_observer_; > >> I don't mind, but was this change necessary? Do we need a pointer to the > >> subtype? > >> > >> > http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifi... > >> File net/base/network_change_notifier.h (right): > >> > >> > http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifi... > >> net/base/network_change_notifier.h:64: virtual ~OnlineStateObserver() {} > >> These types aren't refcounted, why are their destructors being made > >> protected? > >> > >> http://codereview.chromium.org/10417002/
On Mon, May 21, 2012 at 11:58 AM, <rsleevi@chromium.org> wrote: > On 2012/05/21 17:54:06, willchan wrote: > >> Do you think this is a good thing to do in general? Or only when you >> multiply inherit? I'd tentatively lean towards saying that this is not a >> good thing to do in general, and the problem we run into with multiple >> inheritance is a code smell that indicates we're doing something wrong. >> Probably because multiple inheritance is stupid :) >> > > We use multiple inheritance heavily, but for what are intended to be pure > virtual interface classes. While there should only be one 'concrete' base > class, > by the time you make it to the content/ and chrome/ layers (or even the > media/ > or sync/ layers), M-I abounds. I believe you are stating that M-I is a fact of life in those code bases, not that it's a good thing. I'm arguing it is indeed not a good thing (I'm probably in the minority). I think most people don't want to write the extra boilerplate to get the inheritance hierarchy "right". > > > > Can we solve this by getting rid of the multiple inheritance instead? How >> about a refcounted class that wraps these base classes you're changing? I >> don't like losing the polymorphic destruction, as that makes dependency >> injection harder/uglier. >> > > For situations where the lifetime is ambiguous, or where polymorphic > destruction > is actually a desired trait, this is exactly what I've been recommending. > I think polymorphic destruction is in general a desired trait for dependency injection reasons. We're throwing that power away in many of your changes (where we admittedly weren't using DI anyway). I find it sad because we're changing base classes to accommodate individual derived classes, and now all the other derived classes who may have wanted to use DI will not be able to use it. > > In most cases, however, the polymorphic destruction is not intended - the > lifetime contracts are such that "Delegates supplied to this > [class/function] > must outlive this [class/function]", and so making the destructor protected > simply guarantees that. > > > > Related to this discussion here is >> http://www.gotw.ca/**publications/mill18.htm<http://www.gotw.ca/publications/... guideline 4 says: "A base >> class destructor should be either public and virtual, or protected and >> nonvirtual." >> > > Yes, and that's certainly my thought on it, but our style guide & clang > checker > warn if you have virtual methods on a class without a virtual destructor. > If we really wanted to be explicit, yes, such pure virtual interfaces > should > have concrete, non-virtual, protected destructors, since there never > should be a > polymorphic delete. I don't think it's been worth the (possible, > speculative) > compile time hit to make the existing style checker inspect the class to > see if > it's all pure virtual methods. > Fair enough. > > > On Mon, May 21, 2012 at 8:19 AM, Ryan Sleevi <mailto:rsleevi@chromium.org >> > >> > wrote: > >> > >> > Sorry, I should have been more explicit. These are all /base/ classes >> for >> multiply-inheriting RefCounted classes. >> > >> > Unless we explicitly need ownership of the Delegate, keep the >> destructors >> protected so that their lifetimes are ambiguous at this layer - and to >> prevent anyone from doing scoped_ptr<Delegate> foo and then sticking the >> ref counted implementation inside. >> > >> > Make sense? >> > >> > On May 21, 2012 7:31 AM, <mailto:willchan@chromium.org> wrote: >> >> >> >> Wait, I just started going through this. These types aren't >> refcounted...what's >> >> the change for? >> >> >> >> >> >> http://codereview.chromium.**org/10417002/diff/1/chrome/** >> browser/io_thread.h<http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h> >> >> File chrome/browser/io_thread.h (right): >> >> >> >> >> > > http://codereview.chromium.**org/10417002/diff/1/chrome/** > browser/io_thread.h#newcode188<http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h#newcode188> > >> >> chrome/browser/io_thread.h:**188: scoped_ptr<** >> LoggingNetworkChangeObserver> >> >> network_change_observer_; >> >> I don't mind, but was this change necessary? Do we need a pointer to >> the >> >> subtype? >> >> >> >> >> > > http://codereview.chromium.**org/10417002/diff/1/net/base/** > network_change_notifier.h<http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifier.h> > >> >> File net/base/network_change_**notifier.h (right): >> >> >> >> >> > > http://codereview.chromium.**org/10417002/diff/1/net/base/** > network_change_notifier.h#**newcode64<http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifier.h#newcode64> > >> >> net/base/network_change_**notifier.h:64: virtual >> ~OnlineStateObserver() {} >> >> These types aren't refcounted, why are their destructors being made >> >> protected? >> >> >> >> http://codereview.chromium.**org/10417002/<http://codereview.chromium.org/104... >> > > > > http://codereview.chromium.**org/10417002/<http://codereview.chromium.org/104... >
On Mon, May 21, 2012 at 12:08 PM, William Chan (陈智昌) <willchan@chromium.org>wrote: > On Mon, May 21, 2012 at 11:58 AM, <rsleevi@chromium.org> wrote: > >> On 2012/05/21 17:54:06, willchan wrote: >> >>> Do you think this is a good thing to do in general? Or only when you >>> multiply inherit? I'd tentatively lean towards saying that this is not a >>> good thing to do in general, and the problem we run into with multiple >>> inheritance is a code smell that indicates we're doing something wrong. >>> Probably because multiple inheritance is stupid :) >>> >> >> We use multiple inheritance heavily, but for what are intended to be pure >> virtual interface classes. While there should only be one 'concrete' base >> class, >> by the time you make it to the content/ and chrome/ layers (or even the >> media/ >> or sync/ layers), M-I abounds. > > > I believe you are stating that M-I is a fact of life in those code bases, > not that it's a good thing. I'm arguing it is indeed not a good thing (I'm > probably in the minority). I think most people don't want to write the > extra boilerplate to get the inheritance hierarchy "right". > Well, once it's a compile check, it's write once, think never. If you ever screw up the inheritance hierarchy, the bots will start complaining and refuse to compile. And I think that's a huge step forward from where we are, which absolutely requires careful thought and inspection to see if it might be an issue. > >> >> >> Can we solve this by getting rid of the multiple inheritance instead? How >>> about a refcounted class that wraps these base classes you're changing? I >>> don't like losing the polymorphic destruction, as that makes dependency >>> injection harder/uglier. >>> >> >> For situations where the lifetime is ambiguous, or where polymorphic >> destruction >> is actually a desired trait, this is exactly what I've been recommending. >> > > I think polymorphic destruction is in general a desired trait for > dependency injection reasons. We're throwing that power away in many of > your changes (where we admittedly weren't using DI anyway). I find it sad > because we're changing base classes to accommodate individual derived > classes, and now all the other derived classes who may have wanted to use > DI will not be able to use it. > I don't think it prevents DI. It enshrines in code what the current DI policy is - that you /can/ inject the dependency, but it's lifetime/ownership is not yours to maintain. You can still hold a pointer to the interface, just don't delete it. If these classes do want to do polymorphic destruction / if they need to manage the lifetime, they can promote the destructor from protected -> public. At that time, if there are any RefCounted implementations, they'll get the compile warning about the unmet expectations, and can resolve by splitting the RefCounted implementations. Alternatively (and admittedly, this is a readability hit), they can split into Interface & Base, where Base has a public destructor but Interface declares the pure virtual methods. Then implementations which do want to maintain ownership can use the Base, while those that do not can continue to use/supply the Interface. If you ever try to supply an Interface (non-owning) to something that expects a Base (owning), you'd get a compile error there. Either way though, I'd rather it be a compile error if some derived implementation does want to own it, while some other derived implementation wants to be refcounted. An example of exactly what you're talking about is http://crbug.com/128942 > > >> >> In most cases, however, the polymorphic destruction is not intended - the >> lifetime contracts are such that "Delegates supplied to this >> [class/function] >> must outlive this [class/function]", and so making the destructor >> protected >> simply guarantees that. >> >> >> >> Related to this discussion here is >>> http://www.gotw.ca/**publications/mill18.htm<http://www.gotw.ca/publications/... guideline 4 says: "A base >>> class destructor should be either public and virtual, or protected and >>> nonvirtual." >>> >> >> Yes, and that's certainly my thought on it, but our style guide & clang >> checker >> warn if you have virtual methods on a class without a virtual destructor. > > >> If we really wanted to be explicit, yes, such pure virtual interfaces >> should >> have concrete, non-virtual, protected destructors, since there never >> should be a >> polymorphic delete. I don't think it's been worth the (possible, >> speculative) >> compile time hit to make the existing style checker inspect the class to >> see if >> it's all pure virtual methods. >> > > Fair enough. > > >> >> >> On Mon, May 21, 2012 at 8:19 AM, Ryan Sleevi <mailto: >>> rsleevi@chromium.org> >>> >> wrote: >> >>> > >>> > Sorry, I should have been more explicit. These are all /base/ classes >>> for >>> multiply-inheriting RefCounted classes. >>> > >>> > Unless we explicitly need ownership of the Delegate, keep the >>> destructors >>> protected so that their lifetimes are ambiguous at this layer - and to >>> prevent anyone from doing scoped_ptr<Delegate> foo and then sticking the >>> ref counted implementation inside. >>> > >>> > Make sense? >>> > >>> > On May 21, 2012 7:31 AM, <mailto:willchan@chromium.org> wrote: >>> >> >>> >> Wait, I just started going through this. These types aren't >>> refcounted...what's >>> >> the change for? >>> >> >>> >> >>> >> http://codereview.chromium.**org/10417002/diff/1/chrome/** >>> browser/io_thread.h<http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h> >>> >> File chrome/browser/io_thread.h (right): >>> >> >>> >> >>> >> >> http://codereview.chromium.**org/10417002/diff/1/chrome/** >> browser/io_thread.h#newcode188<http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h#newcode188> >> >>> >> chrome/browser/io_thread.h:**188: scoped_ptr<** >>> LoggingNetworkChangeObserver> >>> >> network_change_observer_; >>> >> I don't mind, but was this change necessary? Do we need a pointer to >>> the >>> >> subtype? >>> >> >>> >> >>> >> >> http://codereview.chromium.**org/10417002/diff/1/net/base/** >> network_change_notifier.h<http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifier.h> >> >>> >> File net/base/network_change_**notifier.h (right): >>> >> >>> >> >>> >> >> http://codereview.chromium.**org/10417002/diff/1/net/base/** >> network_change_notifier.h#**newcode64<http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifier.h#newcode64> >> >>> >> net/base/network_change_**notifier.h:64: virtual >>> ~OnlineStateObserver() {} >>> >> These types aren't refcounted, why are their destructors being made >>> >> protected? >>> >> >>> >> http://codereview.chromium.**org/10417002/<http://codereview.chromium.org/104... >>> >> >> >> >> http://codereview.chromium.**org/10417002/<http://codereview.chromium.org/104... >> > >
I VC'd with rsleevi to discuss this. Ryan agreed that removing public destructors makes DI annoying since then you need to derive from a derived class with a public destructor. That said, there were very few cases where we wanted to do that anyway, and my preferred solution of eliminating the multiple inheritance is kludgy. So, despite the uncleanliness of this approach, <holdnose>LGTM</holdnose>. On Mon, May 21, 2012 at 12:18 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > > > On Mon, May 21, 2012 at 12:08 PM, William Chan (陈智昌) < > willchan@chromium.org> wrote: > >> On Mon, May 21, 2012 at 11:58 AM, <rsleevi@chromium.org> wrote: >> >>> On 2012/05/21 17:54:06, willchan wrote: >>> >>>> Do you think this is a good thing to do in general? Or only when you >>>> multiply inherit? I'd tentatively lean towards saying that this is not a >>>> good thing to do in general, and the problem we run into with multiple >>>> inheritance is a code smell that indicates we're doing something wrong. >>>> Probably because multiple inheritance is stupid :) >>>> >>> >>> We use multiple inheritance heavily, but for what are intended to be pure >>> virtual interface classes. While there should only be one 'concrete' >>> base class, >>> by the time you make it to the content/ and chrome/ layers (or even the >>> media/ >>> or sync/ layers), M-I abounds. >> >> >> I believe you are stating that M-I is a fact of life in those code bases, >> not that it's a good thing. I'm arguing it is indeed not a good thing (I'm >> probably in the minority). I think most people don't want to write the >> extra boilerplate to get the inheritance hierarchy "right". >> > > Well, once it's a compile check, it's write once, think never. If you ever > screw up the inheritance hierarchy, the bots will start complaining and > refuse to compile. And I think that's a huge step forward from where we > are, which absolutely requires careful thought and inspection to see if it > might be an issue. > > >> >>> >>> >>> Can we solve this by getting rid of the multiple inheritance instead? >>>> How >>>> about a refcounted class that wraps these base classes you're changing? >>>> I >>>> don't like losing the polymorphic destruction, as that makes dependency >>>> injection harder/uglier. >>>> >>> >>> For situations where the lifetime is ambiguous, or where polymorphic >>> destruction >>> is actually a desired trait, this is exactly what I've been recommending. >>> >> >> I think polymorphic destruction is in general a desired trait for >> dependency injection reasons. We're throwing that power away in many of >> your changes (where we admittedly weren't using DI anyway). I find it sad >> because we're changing base classes to accommodate individual derived >> classes, and now all the other derived classes who may have wanted to use >> DI will not be able to use it. >> > > I don't think it prevents DI. It enshrines in code what the current DI > policy is - that you /can/ inject the dependency, but it's > lifetime/ownership is not yours to maintain. You can still hold a pointer > to the interface, just don't delete it. > > If these classes do want to do polymorphic destruction / if they need to > manage the lifetime, they can promote the destructor from protected -> > public. At that time, if there are any RefCounted implementations, they'll > get the compile warning about the unmet expectations, and can resolve by > splitting the RefCounted implementations. > > Alternatively (and admittedly, this is a readability hit), they can split > into Interface & Base, where Base has a public destructor but Interface > declares the pure virtual methods. Then implementations which do want to > maintain ownership can use the Base, while those that do not can continue > to use/supply the Interface. If you ever try to supply an Interface > (non-owning) to something that expects a Base (owning), you'd get a compile > error there. > > Either way though, I'd rather it be a compile error if some derived > implementation does want to own it, while some other derived implementation > wants to be refcounted. An example of exactly what you're talking about is > http://crbug.com/128942 > > >> >> >>> >>> In most cases, however, the polymorphic destruction is not intended - the >>> lifetime contracts are such that "Delegates supplied to this >>> [class/function] >>> must outlive this [class/function]", and so making the destructor >>> protected >>> simply guarantees that. >>> >>> >>> >>> Related to this discussion here is >>>> http://www.gotw.ca/**publications/mill18.htm<http://www.gotw.ca/publications/... guideline 4 says: "A base >>>> class destructor should be either public and virtual, or protected and >>>> nonvirtual." >>>> >>> >>> Yes, and that's certainly my thought on it, but our style guide & clang >>> checker >>> warn if you have virtual methods on a class without a virtual destructor. >> >> >>> If we really wanted to be explicit, yes, such pure virtual interfaces >>> should >>> have concrete, non-virtual, protected destructors, since there never >>> should be a >>> polymorphic delete. I don't think it's been worth the (possible, >>> speculative) >>> compile time hit to make the existing style checker inspect the class to >>> see if >>> it's all pure virtual methods. >>> >> >> Fair enough. >> >> >>> >>> >>> On Mon, May 21, 2012 at 8:19 AM, Ryan Sleevi <mailto: >>>> rsleevi@chromium.org> >>>> >>> wrote: >>> >>>> > >>>> > Sorry, I should have been more explicit. These are all /base/ classes >>>> for >>>> multiply-inheriting RefCounted classes. >>>> > >>>> > Unless we explicitly need ownership of the Delegate, keep the >>>> destructors >>>> protected so that their lifetimes are ambiguous at this layer - and to >>>> prevent anyone from doing scoped_ptr<Delegate> foo and then sticking the >>>> ref counted implementation inside. >>>> > >>>> > Make sense? >>>> > >>>> > On May 21, 2012 7:31 AM, <mailto:willchan@chromium.org> wrote: >>>> >> >>>> >> Wait, I just started going through this. These types aren't >>>> refcounted...what's >>>> >> the change for? >>>> >> >>>> >> >>>> >> http://codereview.chromium.**org/10417002/diff/1/chrome/** >>>> browser/io_thread.h<http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h> >>>> >> File chrome/browser/io_thread.h (right): >>>> >> >>>> >> >>>> >>> >>> http://codereview.chromium.**org/10417002/diff/1/chrome/** >>> browser/io_thread.h#newcode188<http://codereview.chromium.org/10417002/diff/1/chrome/browser/io_thread.h#newcode188> >>> >>>> >> chrome/browser/io_thread.h:**188: scoped_ptr<** >>>> LoggingNetworkChangeObserver> >>>> >> network_change_observer_; >>>> >> I don't mind, but was this change necessary? Do we need a pointer to >>>> the >>>> >> subtype? >>>> >> >>>> >> >>>> >>> >>> http://codereview.chromium.**org/10417002/diff/1/net/base/** >>> network_change_notifier.h<http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifier.h> >>> >>>> >> File net/base/network_change_**notifier.h (right): >>>> >> >>>> >> >>>> >>> >>> http://codereview.chromium.**org/10417002/diff/1/net/base/** >>> network_change_notifier.h#**newcode64<http://codereview.chromium.org/10417002/diff/1/net/base/network_change_notifier.h#newcode64> >>> >>>> >> net/base/network_change_**notifier.h:64: virtual >>>> ~OnlineStateObserver() {} >>>> >> These types aren't refcounted, why are their destructors being made >>>> >> protected? >>>> >> >>>> >> http://codereview.chromium.**org/10417002/<http://codereview.chromium.org/104... >>>> >>> >>> >>> >>> http://codereview.chromium.**org/10417002/<http://codereview.chromium.org/104... >>> >> >> >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10417002/1
Try job failure for 10417002-1 (retry) (retry) on win_rel for steps "base_unittests, sync_unit_tests" (clobber build). It's a second try, previously, steps "base_unittests, sync_unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10417002/1
Failed to apply patch for content/public/common/url_fetcher_delegate.h: While running patch -p1 --forward --force; patching file content/public/common/url_fetcher_delegate.h Hunk #1 FAILED at 18. 1 out of 1 hunk FAILED -- saving rejects to file content/public/common/url_fetcher_delegate.h.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10417002/17001
Change committed as 139272 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
