Overriding public virtual methods as private in C++?

[Ryan Landay]

Hello,

I have a CL up for review where I'm implementing virtual methods on a derived class:

https://codereview.chromium.org/2772423002/#msg15

The virtual methods are declared as public on the base class. My understanding was previously that the implementations in the derived classes should also be declared as public. However, it came up during review that there's apparently a pattern in Chromium code of declaring the derived methods as private, I guess to force people to declare variable types using the base class instead of the derived class.

This seems weird to me because the derived classes I'm defining have some methods specific to them, so there are instances where we actually need to have a variable declared as a pointer to a derived class, in which case, it'd be impossible to call these overridden public methods without upcasting.

I haven't been able to find anything about this rule in the Google C++ or Chromium style guides, and at least one source on the internet highly discourages overriding public virtual methods as private:

https://isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public

Is this a style convention we use in Chromium (or maybe just in Blink)? If so, what's the reasoning behind it?

Thanks,

Ryan

[Peter Kasting]

I'm one of the proponents of doing this in certain cases, so I can speak some to this.

Basically, declare such overrides as public if people will actually have pointers to your derived type for other reasons and legitimately want to call these methods through them.

You can declare the overrides as private when your type is effectively an implementation detail and consumers would not be expected to access these methods through a pointer to the derived type, and you're trying to enforce that the base type is the "public API".  For example, in the infobar system, callers should be working with core types like InfoBarDelegate rather than the specific delegate subclasses that implement particular infobars, so generally those delegates do private overrides.

A rule of thumb is that if you ever legitimately have to cast from a derived pointer to a base pointer just to call a method, that method likely should not have been a private override.  Definitely do not just always make all virtual methods private in all derived classes.

PK

[Peter Kasting]

That said, general sentiment within Google seems to be largely negative on this, so I'm willing to believe I'm just wrong to have advocated for this.  The core argument against this is that since inheritance is meant to model "is-a", privatizing derived methods goes against the concept that the derived type "is-a" base type, because it can no longer be used like the base type.  If hiding these is useful to prevent access to other methods in the derived type that are unrelated to the base type, perhaps the presence of those methods indicates the derived type is too many things at once and should be using composition rather than inheritance.

So either this "protection" isn't actually buying anything real, in which case remove it, or it is, in which case it's a strong signal that the design should be changed.

Conclusion: expose derived methods using the same level of access as the base methods.

Anyone want to argue this?  :)

PK

[Devlin Cronin]

I think probably the most common case where I find reducing the level of access of inherited methods is in observers.  Consider:

class FooObserver {

 public:

  virtual void OnSomethingHappened() = 0;

};

class SomethingInterestedInFoos : public FooObserver {

 public:

  ....

 private:

  void OnSomethingHappened() override;

};

It's very rare that anything should be calling SomethingInterestedInFoos::OnSomethingHappened() directly; rather, it should usually only be accessed by things that are interested in FooObservers (likely Foos).  And typically something calling OnSomethingHappened() directly on a SomethingInterestedInFoos is an indication that something is wrong.

We *could* say that this is questionable design, and offer the alternative:

class SomethingInterestedInFoos {

 public:

  ....

 private:

  class FooObserverHelper : public FooObserver {

   public:

    FooObserverHelper(SomethingInterestedInFoos* owner) : owner_(owner) {}

    void OnSomethingHappened() override { owner_->OnSomethingHappened(); }

   private:

    SomethingInterestedInFoos* owner_ = nullptr;

  };

  void OnSomethingHappened();

  FooObserverHelper observer_helper_;

}

But at the same time, that's an awful lot of redirection to be able to satisfy the joint desire for a) not reducing level of access and b) not allowing things to call observer methods on direct objects.

I tend to think that, like many things, reducing level of access is acceptable and desirable in certain cases, and not in others, and I'd be happy to leave it up to reviewers.

Just my $0.02.

Cheers,

- Devlin

[Peter Kasting]

On Wed, Mar 29, 2017 at 6:32 PM, Devlin Cronin <rdevlin...@chromium.org> wrote:

I think probably the most common case where I find reducing the level of access of inherited methods is in observers.  Consider:

class FooObserver {

 public:

  virtual void OnSomethingHappened() = 0;

};

class SomethingInterestedInFoos : public FooObserver {

 public:

  ....

 private:

  void OnSomethingHappened() override;

};

It's very rare that anything should be calling SomethingInterestedInFoos::OnSomethingHappened() directly; rather, it should usually only be accessed by things that are interested in FooObservers (likely Foos).  And typically something calling OnSomethingHappened() directly on a SomethingInterestedInFoos is an indication that something is wrong.

Yeah, the internal thread discussed this case, but they basically concluded it's better to just be public most of the time, and if you really REALLY need to prevent people accessing this, use composition as in your (elided) proposal, which will actually achieve that end.

PK

[Marc Treib]

I'm with Devlin and Peter on this: IMO the nested ObserverHelper class is too much indirection. OTOH, making the overridden observer methods public forces the reader of the class interface to skip over uninteresting stuff. So making them private is IMO often the best tradeoff - a poor man's private inheritance (which is banned by the style guide).

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFDnJfBqTQD_bpmefrJwsCz-ih4stuJAMbSAUpwJLfC7yQ%40mail.gmail.com.

[Elliott Sprehn]

We often hide type checking methods in blink since using them on the correct type already doesn't make sense. Ex. If you already have a foo doing foo->isFoo() doesn't make sense. We found this caught a bunch of silly code. 

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFA9i10ui5htrkLLyBrnV6xcw1BUUJ-_xNbbNsoDX%2BcdWQ%40mail.gmail.com.

[Adam Rice]

We often hide type checking methods in blink since using them on the correct type already doesn't make sense. Ex. If you already have a foo doing foo->isFoo() doesn't make sense. We found this caught a bunch of silly code. 

But in general this runs afoul of https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_ (Blink has compelling reasons why it needs to do this, but it should not be general practice in Chrome).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAO9Q3i%2BwHP-wmbU9%2BAoGjYo0Vj4n_aAP7rgEUOxmz2z-y63Tvg%40mail.gmail.com.

[Tim]

Initially I didn't have too strong of an opinion about this topic.

Having seen more examples in our code base, I actually realize it's often a symptom of a unnecessary code complexity when used with multiple inheritance.

So, why is it that you want to hide those methods from the interface?

• If it's because there are so many methods on it, and the class is actually doing other things, you should not use inheritance but composition (here's one example: https://codereview.chromium.org/2558393004/ -- as you can see the resulting CachedImageFetcher class still uses the visibility hiding trick, but in this case it's totally fine to make them public).
  This seems a widespread problem within Chrome and that's why i'm more having a stronger opinion against that trick.
• Otherwise, chances are high the class is focused on one particular thing (including the implemented interface) and then a comment explaining the methods would do as fine IMO.  No need to (ineffectively trying to) hide those methods at all. Examples for such classes can be adaptors that listen to some events and propagate the signals to other components.

Even observers might be problematic:

Oberservers can observe many things and in many cases the class that needs access to the observed information is only interested in very specific data. So instead of having the class to deal with the full observer semantics and complexity, introducing a simpler interface that only provides the required data is actually desirable. When introducing a separate class implementing the  observer interface (like the FooObserverHelper but maybe with a meaningful name ;-)), that FooObserverHelper becomes an adaptor to such a simplified interface. The result is a much cleaner separation of responsibility, lower coupling and lower complexity.

Here's an example with observers where I believe such a refactoring would result in code that|s easier to understand/maintain and much easier to test:

https://codereview.chromium.org/2629603002/diff/1/chrome/browser/ntp_snippets/download_suggestions_provider.h#newcode42

--Tim