[chromium-dev] Rule about overring public method?

[Simon Hong]

Hi,

For example, there are many child classes that inherits WebContentsObserver.

There are two cases.

One is overriding methods as a public method.

The other is as a private method.

Which is the right way?

Thank you.

Simon.

[Peter Kasting]

I'm not sure I understand.  Are you asking what visibility child classes should give to overridden methods declared in a parent class?  If so, the codebase is not consistent, and there are a couple of different views, though honestly the difference is not very large, so this is something of a bikeshed.

The two main viewpoints are (1) use the same visibility as the parent class, and (2) make child methods "as private as possible".

The advantage of (1) is that it's easy to implement and unsurprising -- polymorphism does not change the "publicness" of a method accessed through a pointer.

The advantage of (2) is that you can use it to enforce API correctness in certain cases.  For example, you may have a class that inherits from NotificationObserver.  It's usually wrong to call Observe() directly on such a class, as Observe() should normally only be called by the notification system.  Making Observe() private in the class enforces this.

Personally I make a habit of doing (2) (since I'd rather always do one or the other, and I think the benefits of (2) are larger), but as I said, there is not broad agreement, and no style rule covers this.

PK

[Simon Hong]

I just wondered two cases are mix-used even with same parent.

Yes, I also agree with the advantage of (2).

If guide for this is made, code style should be consistent. 

Thank you!

[Peter Kasting]

We're not going to write a style rule for this.  It's too minor a point to codify, especially since we've more recently gotten less willing to write style rules that go beyond the Google style guide for any reason.

PK

[YoungKi Hong]

Yes, I agree that It's a minor issue to make a new coding rule.

Thank you for kind reply, Peter.

[Ryan Sleevi]

On Thu, Nov 22, 2012 at 12:45 PM, Peter Kasting <pkas...@chromium.org> wrote:
> On Thu, Nov 22, 2012 at 11:56 AM, Simon Hong <simon....@gmail.com> wrote:
>>
>> For example, there are many child classes that inherits
>> WebContentsObserver.
>> There are two cases.
>> One is overriding methods as a public method.
>> The other is as a private method.
>>
>> Which is the right way?
>
>
> I'm not sure I understand. Are you asking what visibility child classes
> should give to overridden methods declared in a parent class? If so, the
> codebase is not consistent, and there are a couple of different views,
> though honestly the difference is not very large, so this is something of a
> bikeshed.
>
> The two main viewpoints are (1) use the same visibility as the parent class,
> and (2) make child methods "as private as possible".
>
> The advantage of (1) is that it's easy to implement and unsurprising --
> polymorphism does not change the "publicness" of a method accessed through a
> pointer.
>
> The advantage of (2) is that you can use it to enforce API correctness in
> certain cases. For example, you may have a class that inherits from
> NotificationObserver. It's usually wrong to call Observe() directly on such
> a class, as Observe() should normally only be called by the notification
> system. Making Observe() private in the class enforces this.

Not really. If you've publicly inherited from NotificationObserver
[1], then Observe() is still available to anyone who has access to
your Foo*. This is because they can simply upcast your Foo* to a
NotificationObserver* and call Observe() on that - perhaps violating
the expectations of your Foo*. This may not be done directly either
(eg: via a static_cast<NotificationObserver*>) - you may simply be
calling a method that expects a NotificationObserver* and the compiler
is doing the casting for you.

I would say that if you're trying to restrict the visibility of a
parent class's methods, you're doing the inheritance wrong, because
you're not really matching the same constraints and pre-/post-
conditions of the parent class. If the parent class says a method is
"Anybody can call this", then your derived class should also be
following the "Anybody can call this" design. If you're trying to
restrict the visibility of that, then in general, composition would
seem the more favourable approach here, based on the style guide.

My experience with bugs related to this pattern ("There's no way
anybody can call this method at the wrong time - it's private!") are
similar to my experiences with multiple inheritance [2] - they are
subtle and easy to miss/hard to spot, so if you find your design
gravitating towards making a base class's public method private in
your derived class, it may be suitable to do a design double-check and
make sure the abstraction is right.

>
> Personally I make a habit of doing (2) (since I'd rather always do one or
> the other, and I think the benefits of (2) are larger), but as I said, there
> is not broad agreement, and no style rule covers this.
>
> PK
>

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

[1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheritance#Inheritance
[2] https://groups.google.com/a/chromium.org/d/topic/chromium-dev/mya3DMQkHWA/discussion

[Peter Kasting]

On Mon, Nov 26, 2012 at 10:11 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

If you've publicly inherited from NotificationObserver

[1], then Observe() is still available to anyone who has access to
your Foo*. This is because they can simply upcast your Foo* to a
NotificationObserver* and call Observe() on that

Yes, I realize that.  In fact I actually take advantage of this in one or two places where, for example, I need to call a method directly from a test but nowhere else, so I have the test upcast.

Still, this speedbump is enough to prevent most cases of this kind of usage, which for me is Good Enough.  There are plenty of other cases where we have objects or APIs that _can_ be misused but are easier to use correctly (e.g. scoped_ptr), and we consider those wins.

As for calling methods that expect a Parent* -- that's precisely how this is supposed to be working.  The whole point is that people should be able to call these, they just should be calling through a Parent*.

My experience with bugs related to this pattern ("There's no way
anybody can call this method at the wrong time - it's private!") are
similar to my experiences with multiple inheritance [2] - they are
subtle and easy to miss/hard to spot, so if you find your design
gravitating towards making a base class's public method private in
your derived class, it may be suitable to do a design double-check and
make sure the abstraction is right.

As I noted before, I simply always make derived members as private as possible, rather than just trying to anticipate where there would be particular wins and do that only there.  So it's not generally a question of a design gravitating toward this.

I still think there is something to be said for a header being able to tell you that a method is only called via a parent pointer, not directly on the child object.  Maybe that's not useful knowledge to other people, but it is to me.

PK

[Brett Wilson]

I like doing this as well. It's nice to be able to look at the header
and see the public API of the object that you're expected to use.

I might like to use private inheritance for some things like
NotificationObserver, but the style guide prohibits that, so this hint
seems useful.

Brett