making public methods private in a derived class

[Paweł Hajdan, Jr.]

This came up during review of https://codereview.chromium.org/12212089/

Is it a new recommendation to make public virtual methods inherited from an interface private in the deriving class?

I couldn't find anything on http://dev.chromium.org/developers/coding-style or http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts

However, I could find some negatives on http://stackoverflow.com/questions/484592/overriding-public-virtual-functions-with-private-functions-in-c and http://www.parashift.com/c++-faq-lite/hiding-inherited-public.html

For example, it's still possible to access the "now-private" methods by upcasting to the interface class. Now OK, a cast like that looks weird, but if we go that route (to make inherited public methods private) it should be discussed, documented, and followed consistently IMHO.

What do you think?

[Nico Weber]

Without looking at any of these links, doesn't narrowing visibility
violate the liskov substitution principle? I thought making a method
more visible in a child class is fine, while the reverse isn't.
(Likewise, return types are contravariant in some languages while
parameter types are covariant, etc.)

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
>
>
>

[Jói Sigurðsson]

Since a B inheriting from A and hiding some of A's methods still is-an
A, I don't think it violates the principle. You can still use it as an
A as long as you hold an A pointer to it.

I think there already was a thread on chromium-dev, not too long ago,
discussing the pros and cons of making inherited methods less public.
I may be wrong.

Cheers,
Jói

[Bartosz Fabianowski]

In my experience, this is a controversial topic that some people feel
very strongly about. I have had reviewers ask me to hide inherited
methods in derived classes before. And I have had reviewers ask me not
to do it. I do not like hiding methods like that but I will do it if a
reviewer feels strongly.

- Bartosz

[Anton Vayvod]

FTR, the thread mentioned above: https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-dev/MkBEHBx3g_4

Since this came up again and is somewhat confusing, maybe we need a style guide rule after all? This is probably not covered by Google style guide because multiple inheritance of interfaces is not common at Google, wide usage of delegates and observers is a more Chrome-specific thing.

It seems that hiding public interface (delegate, observer) implementation is a win since it enforces the interface API and keeps the public interface of the implementation class short.

Generic inheriting from a non-interface class should probably stick to keeping the parent's methods visibility since that what a client of the class would expect.

[Thiago Farina]

On Thu, Feb 21, 2013 at 11:55 AM, Anton Vayvod <ava...@chromium.org> wrote:
> FTR, the thread mentioned above:
> https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-dev/MkBEHBx3g_4
> Since this came up again and is somewhat confusing, maybe we need a style
> guide rule after all? This is probably not covered by Google style guide
> because multiple inheritance of interfaces is not common at Google, wide
> usage of delegates and observers is a more Chrome-specific thing.
> It seems that hiding public interface (delegate, observer) implementation is
> a win since it enforces the interface API and keeps the public interface of
> the implementation class short.

Yes, that is good explanation and I have always followed that! You
separate the public API of the Interface (base) from the public API of
the derived class.

Most of the time you are supposed to do this:

Interface* interface = CreateInterface();

class InterfaceImpl : public Interface {
};

interface->Foo();

Not:

InterfaceImpl* impl = CreateInterface();
impl->Foo();

Hence making the implementation of the methods of the interface in the
derived class private works, i.e, you call it through the interface
(base class), not the derived one.

--
Thiago

[Paweł Hajdan, Jr.]

On Thu, Feb 21, 2013 at 6:55 AM, Anton Vayvod <ava...@chromium.org> wrote:

FTR, the thread mentioned above: https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-dev/MkBEHBx3g_4

Since this came up again and is somewhat confusing, maybe we need a style guide rule after all? This is probably not covered by Google style guide because multiple inheritance of interfaces is not common at Google, wide usage of delegates and observers is a more Chrome-specific thing.

Right, can we get some decision about this? IMO the worst outcome is just inconsistent application of conflicting rules across the codebase. This should be predictable and not depend on who's reviewing the code.

Paweł

[Thiago Farina]

On Tue, Feb 26, 2013 at 3:20 PM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> Right, can we get some decision about this? IMO the worst outcome is just
> inconsistent application of conflicting rules across the codebase.

I think that already happened, because the team is so large as is the
codebase and there are differing preferences among OWNERS. Some follow
some rules, other don't know some of them and so on.

--
Thiago

[Wez]

In the absence of other pros/cons, my 2c is that the pro (shorter public interfaces with clear separation between the class' interface and stuff it just happens to implement to get its job done) is more valuable to code readability.

Wez

[Darin Fisher]

On Thu, Feb 21, 2013 at 6:55 AM, Anton Vayvod <ava...@chromium.org> wrote:

FTR, the thread mentioned above: https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-dev/MkBEHBx3g_4

Since this came up again and is somewhat confusing, maybe we need a style guide rule after all? This is probably not covered by Google style guide because multiple inheritance of interfaces is not common at Google, wide usage of delegates and observers is a more Chrome-specific thing.

It seems that hiding public interface (delegate, observer) implementation is a win since it enforces the interface API and keeps the public interface of the implementation class short.

^^^ This.

Unfortunately (or fortunately?) the Google style guide forbids private inheritance.  That means, if your class needs to implement an interface as an implementation detail, you are left exposing that implementation detail to consumers of your class.  This seems ugly.

As a means of making the intended public interface of my class more apparent, I usually hide the implementation of such interfaces in the private section of my class.  Even though it doesn't prevent those methods from being called, it at least means that consumers have to consciously cast to the interface type in order to call the method.  Accidental abuse of that seems rare.

I'm not sure this warrants an addition to our style guide.  It seems like a minor style point.

-Darin

[Drew Wilson]

As an aside, here's what the style guide says about private inheritance:

All inheritance should be public. If you want to do private inheritance, you should be including an instance of the base class as a member instead.

What does that mean, exactly, in the case of Observer interfaces? That we should all add boilerplate "observer" internal classes that translate invocations of the Observer methods into invocations of private methods on the public class? It seems like that's the expectation here, in which case the real problem is that we've all been Doing It Wrong, and all the public interface inheritance littering the chrome codebase is actually a code smell?

I guess I've just never really thought about what the "right" way to do things would be from a style-guide perspective.

-atw

[Greg Thompson]

On Wed, Feb 27, 2013 at 5:49 AM, Drew Wilson <atwi...@chromium.org> wrote:

As an aside, here's what the style guide says about private inheritance:

All inheritance should be public. If you want to do private inheritance, you should be including an instance of the base class as a member instead.

I think the spirit of this statement is to favor composition over inheritance when you're using the implementation of some other class. This is a Truth of good design, and I agree that using private inheritance to get another class's behaviors and/or features is wrong. I think Chromium's delegate interfaces are a different beast. In their case, my preferences are (in order):

1. Allow private inheritance.

2. Stick with public inheritance with private methods.

3. Require the use of a private class implementing the delegate interface that shuttles the relevant methods over to the receiver.

I've contorted myself into using #3 a few times, and I find that it overly complicates the code for little benefit. I accept #2 since it follows the letter of the style-law, but I don't see the downside to #1.

To reiterate, I'm talking strictly about delegate interfaces.

Cheers,

Greg

[Scott Hess]

That sounds like it's refering to private inheritance of implementation, it wouldn't even make sense to have a member instance of a base class with pure virtual functions.

-scott

[Anthony Berent]

There is a good description of the need for private inheritance in these circumstances on the C++ FAQ: http://www.parashift.com/c++-faq/priv-inherit-vs-compos.html

- Anthony

[David Michael]

In a lot of cases where you might define a very small delegate, you could instead use base::Callback.

Obviously this is not a good solution when you have a lot of methods, and of course we have tons of existing delegate classes. But using base::Callback lets callback implementers hide their implementation details more readily.

Here's an example CL that changed a 1-member delegate to base::Callback:

https://chromiumcodereview.appspot.com/12094106/diff/25001/base/synchronization/waitable_event_watcher.h

[Tommy Nyquist]

As a side note, we can not apply private inheritance of interfaces across all of our code base, as it is not possible to reduce visibility when implementing an interface in Java (ie. Android).

[Rachel Blum]

I'm puzzled. If you have an Observer, the API _better_ be public. How else is the subject of the observation going to call it? (Yes, of course you can downcast it. I happen to think that that's dirty. It violates the LSP, amongst other things)

In general, non-public interface inheritance seems odd, because it reduces visibility for an interface the class is supposed to expose.

 - rachel

On Wed, Feb 27, 2013 at 2:49 AM, Drew Wilson <atwi...@chromium.org> wrote:

[Anton Vayvod]

On Wed, Feb 27, 2013 at 6:31 PM, Rachel Blum <gr...@chromium.org> wrote:

I'm puzzled. If you have an Observer, the API _better_ be public. How else is the subject of the observation going to call it? (Yes, of course you can downcast it. I happen to think that that's dirty. It violates the LSP, amongst other things)

You need to upcast, not downcast it to the interface. If inheritance is private, only the derived class can upcast itself (and friends?). So it subscribes itself to the observed subject. All users of the derived class don't need to know that it's an observer so they don't call observer methods (except may be for unit tests)

 

In general, non-public interface inheritance seems odd, because it reduces visibility for an interface the class is supposed to expose.

It's rarely used but it's probably there for a reason :) For example, see http://stackoverflow.com/questions/656224/when-should-i-use-c-private-inheritance/675451#675451 and http://www.objectmentor.com/resources/articles/privateInterface.pdf in particular.

[Scott Hess]

On Wed, Feb 27, 2013 at 10:31 AM, Rachel Blum <gr...@chromium.org> wrote:

In general, non-public interface inheritance seems odd, because it reduces visibility for an interface the class is supposed to expose.

For things you are exposing for other code to use against your class, this is true.  But for interfaces you only adhere to for implementation purposes, it makes more sense.  I might be misunderstanding a subtlety in here, but I think of something like:

class SomethingDelegate;

class InterestedParty {

 public:

  void SetDelegate(SomethingDelegate* d);

};

class SomethingDelegate {

 public:

  virtual void DoSomething() = 0;

};

class SomethingDelegateMac : public SomethingDelegate {

 public:

  explicit SomethingDelegateMac(InterestedParty* p);

 private:

  virtual void DoSomething() OVERRIDE;

};

SomethingDelegateMac::SomethingDelegateMac(InterestedParty* p) {

  p->SetDelegate(&this);

}

The "public SomethingDelegate" means that SomethingDelegateMac still is-kind-of SomethingDelegate.  private: SomethingDelegateMac::DoSomething() means that nobody outside of the class implementation (and friends) can call that method directly.  You can only call it via a SomethingDelegate pointer/reference vtable.

I think.

---

I think the private-versus-composition case is where you do something like:

class MyClass : private OtherClass {

};

In this case, MyClass is _not_ kind-of OtherClass, but it can internally use public and protected things OtherClass exposes.  It's a poorly phrased has-a relationship.  I don't think anyone is suggesting that we should be doing this in Chromium code.

-scott

[Rachel Blum]

Let's not discuss up vs. down. I still draw root nodes at the bottom :)

As for the example from SO: #1 makes me want to hit somebody. "Expose part of the base class" is a sign that something is _horribly_ wrong. #2 is debatable - I'd say use has-a instead of is-a, then. 3 is "to save myself typing", and involves implementation inheritance. Yuck. Leaves #4, conveniently what we're discussing here.

And while it's true that only the class needs to know that it's an observer, there's nothing wrong with exposing that it is one. If you really want to keep things private, use base::callback. If the observed class insists on not using callback, have a trampoline that converts to base::Callback. Or fix the observed class.

Basically, there's nothing about the current style guide that prevents writing good code. So why deviate from the master sheet (google style guide)? 

I get the desire to hide, especially if a class implements a bunch of APIs - but in that case, why not ClassInterface (without the API inheritance) and ClassImpl (with the API inheritance, but clearly marked as "not for public consumption"?)

 - rachel

[Thiago Farina]

On Wed, Feb 27, 2013 at 4:04 PM, Rachel Blum <gr...@chromium.org> wrote:
> And while it's true that only the class needs to know that it's an observer,
> there's nothing wrong with exposing that it is one. If you really want to
> keep things private, use base::callback. If the observed class insists on
> not using callback, have a trampoline that converts to base::Callback. Or
> fix the observed class.

It isn't that easy as it may sound. Callbacks are fine for one shot
notification, but not when you have more than one.

To make it clear what I think is being discussed here, take the
following example:

class FooView : public views::View,
public views::LinkListener {
public:
// Overridden from views::Listener:
virtual void LinkClicked(Link* source, int event_flags) OVERRIDE;
...
};

Versus:

class FooView : public views::View,
public views::LinkListener {
private:
// Overridden from views::Listener:
virtual void LinkClicked(Link* source, int event_flags) OVERRIDE;
...
};

So the argument in pro of the second is:

You are NOT supposed to do:

FooView* view = new FooView();
view->LinkClicked(...);

The call to LinkClicked() is responsibility of views framework, and
should be called only from views::Link class in most of the
circumstances, hence private usage is better here IMO, because it
isn't really part of the PUBLIC API exposed by FooView, there are more
interesting methods to be exposed to the clients of FooView than
LinkClicked.

--
Thiago

[Anthony Berent]

There are actually, I think, at least four options being discussed:

Option 1 - public class derivation, public members:

class FooView : public views::View,

                        public views::LinkListener {

 public:

  // Overridden from views::Listener:

  virtual void LinkClicked(Link* source, int event_flags) OVERRIDE;

  ...

};

Option 2 - public class derivation, private members:

class FooView : public views::View,

                        public views::LinkListener {

 private:

  // Overridden from views::Listener:

  virtual void LinkClicked(Link* source, int event_flags) OVERRIDE;

  ...

};

Option 3 - private class derivation:

class FooView : public views::View,

                        private views::LinkListener {

 private:

  // Overridden from views::Listener:

  virtual void LinkClicked(Link* source, int event_flags) OVERRIDE;

  ...

};

Option 4 - composition:

class FooView : public views::View {

  private:

  class FooLinkListener : public views::LinkListener {

    public:

    // Overridden from views::Listener:

    virtual void LinkClicked(Link* source, int event_flags) OVERRIDE;

  }

  

  FooLinkListener listener;

  LinkClicked(Link* source, int event_flags) {

    listener.LinkClicked(source, event_flags);

  }

  ...

};

Options 1 and 2 and 4 are, I believe, all allowed by the coding standard (although the coding standard discourages multiple inheritance), whereas option 3 is forbidden. The problem with options 1 and 2 is that they allow a FooView to be (mis)used as a LinkListener, so, even with option 2, the following code is legal outside FooView:

LinkListener * listener = new FooView();

listener->LinkClicked(source, event);

You probably don't want this, and would like to get a compiler error if this code ever appears, but to forbid this you have to use option 3 or 4. Options 3 and 4 will, I think, have identical behaviour, and will generate almost exactly the same code, so the question is

which of these is clearer. Although 4 is longer I think, on balance, it makes it much clearer that FooView is not a LinkListener, and cannot be used as one. As such my preference, in this case, would be for option 4.

- Anthony

[Anton Vayvod]

On Wed, Feb 27, 2013 at 7:04 PM, Rachel Blum <gr...@chromium.org> wrote:

Let's not discuss up vs. down. I still draw root nodes at the bottom :)

As for the example from SO: #1 makes me want to hit somebody. "Expose part of the base class" is a sign that something is _horribly_ wrong. #2 is debatable - I'd say use has-a instead of is-a, then. 3 is "to save myself typing", and involves implementation inheritance. Yuck. Leaves #4, conveniently what we're discussing here.

I referred the answer for the sake of example #4, sorry for not clarifying. The other link covers this case in more details.

 

And while it's true that only the class needs to know that it's an observer, there's nothing wrong with exposing that it is one. If you really want to keep things private, use base::callback. If the observed class insists on not using callback, have a trampoline that converts to base::Callback. Or fix the observed class.

Callbacks would require the class to be a descendant of RefCountedThreadSafe which would probably bring performance penalty and a possibility of circular links? Not to mention more typing and reading through templates syntax. And you need to verify that you're passing the right method as a callback handler.

Basically, there's nothing about the current style guide that prevents writing good code. So why deviate from the master sheet (google style guide)? 

Using interface inheritance is allowed by the style guide. And it's more common in the client/UI code I believe.

 

I get the desire to hide, especially if a class implements a bunch of APIs - but in that case, why not ClassInterface (without the API inheritance) and ClassImpl (with the API inheritance, but clearly marked as "not for public consumption"?)

More boilerplate code to type? I guess the general idea is to let developers type and read less while achieving the same result.