Warn about name collision from base classes.

[Xiyuan Xia]

Hi,

stevenjb just pointed out a name collision problem that I've created in WidgetDelegateView, where we have

class WidgetDelegate {

  ...

  virtual bool HasHitTestMask() const;

  virtual void GetHitTestMask(gfx::Path* mask) const;

  ...

};

class View {

  ...

  virtual bool HasHitTestMask() const;

  virtual void GetHitTestMask(gfx::Path* mask) const;

  ...

};

class WidgetDelegateView : public WidgetDelegate, public View {

...

};

The two sets of methods defines a hit test mask but the former is meant for a widget and the later is for a view. This creates a subtle bug for a WidgetDelegateView when its bounds does not equal to its widget's bounds.

While I am fixing the names, I am surprised that the compiler does not warn about this. Is there a way to turn on such warning so that similar problems does not happen again?

Thanks.

Xiyuan

[Thiago Farina]

On Fri, Sep 28, 2012 at 7:06 PM, Xiyuan Xia <xiy...@chromium.org> wrote:
> Hi,
>
> stevenjb just pointed out a name collision problem that I've created in
> WidgetDelegateView, where we have
>
> class WidgetDelegate {
> ...
> virtual bool HasHitTestMask() const;
> virtual void GetHitTestMask(gfx::Path* mask) const;
> ...
> };
>
> class View {
> ...
> virtual bool HasHitTestMask() const;
> virtual void GetHitTestMask(gfx::Path* mask) const;
> ...
> };
>
> class WidgetDelegateView : public WidgetDelegate, public View {
> ...
> };
>

I also hit in a multiple inheritance bug due to that in
http://codereview.chromium.org/10795005/

There should be a better way to do that so we could get rid of
WidgetDelegateView entirely.

--
Thiago

[Pavel Ivanov]

There's nothing for compiler to warn about here. I don't think
compiler even sees this name clash until methods are used. If
WidgetDelegateView tried to override any of that methods or they were
used through pointer to WidgetDelegateView then compiler would have
given an error. But apparently WidgetDelegateView is only used through
pointers to base classes meaning compiler always knows which method is
supposed to be used in each case.

Pavel

[Ryan Sleevi]

Right, like Pavel said, it only becomes a warning when you attempt to indirect one of the shadowed methods via WidgetDelegateView, which will then cause a warning of the ambiguity about which virtual method is being called. Virtual Inheritance can be used to cause the WDV to know that both implementations are equivalent, but it seems clear that the implementations and pre-conditions/expectations of the WidgetDelegate and the View interfaces are incompatible under certain situations.

This is one of the reasons why the style guide discourages multiple inheritance ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Multiple_Inheritance#Multiple_Inheritance ). Within Chromium land, M-I is unfortunately used with great abandon - and while it's generally safe with various pure-virtual interfaces, subtle problems like this can crop up. In addition to these inconsistencies, one of the other things that can happen are when the lifetimes of two interfaces are incompatible. Examples include when one interface may be ref-counted, another expected to be owned/deleted, or when weak pointers may get involved (as discussed elsewhere on chromium-dev)

One way to break such a multiple inheritance cycle is to actually have methods used to get implementations of the various interfaces - eg: GetWidgetDelegate(), GetView() - and have internal classes that implement the relevant interfaces and cooperate with the parent class. Because such solutions require more thought and care to implement, and "in general" M-I doesn't cause problems, it's rarer to see in Chromium code, but in the end can result in clearer code and fewer violations of pre-conditions.

That said, Clang at least knows when these M-I issues emerge and when methods may be shadowed / names hidden, so it may be possible to make it warn. However, the algorithm currently used is O(n^2) for the number of classes in the hierarchy, so I'd be a little nervous for compile times.

[Victor Khimenko]

I think people just don't understand why multiple inheritance is dangerous - and no wonder. "Multiple inheritance is bad" and "single inheritance is good" is common delusion (note: style guide authors don't share this delusion), but reality is significantly more complex.

C++ mixes two totally unrelated things: interface inheritance (which existed half-century ago under different name because it's essential for most major projects) and implementation inheritance. Worse: it makes safe "extensionable" implementation inheritance very similarly looking to it's extremely dangerous evil twin.

Interface inheritance is *always* safe. Single-inheritance, multiple inheritance - it's all well and good.

Implementation inheritance combined with virtual functions, on the other hand, is goto++: it's more powerful than goto and it's more dangerous then goto. Multiple inheritance in conjunction with the implementation inheritance with virtual functions (which is what the example which started this thread contained) is unmitigable disaster.

Everyone knows the catchphrase "Go To Statement Considered Harmful" and often people try to avoid goto like a plague. But how many of you have actually READ the aforementioned Edsger Dijkstra's article and compared the reasons for why goto is considered harmful with the reasons for why implementation inheritance is considered harmful? I'll cite: "My ... remark is that our intellectual powers are rather geared to master static relationships and that our powers to visualize processes evolving in time are relatively poorly developed. For that reason we should do (as wise programmers aware of our limitations) our utmost best to shorten the conceptual gap between the static program and the dynamic process, to make the correspondence between the program (spread out in text space) and the process (spread out in time) as trivial as possible".

Well, the implementation inheritance can (and usually does!) play the same role as goto. Consider a simple example:

class Base {

  void foo(void) {

     ...

     bar();

     ...

  }

  virtual void bar(void) {

    ...

  }

}

What have you just did? Well, you've inserted RUNTIME-SWITCHABLE GOTO STATEMENT right in the middle of the instruction foo()! Now you can only understand what the function foo for the particular descendant of the class Base is doing if you'll take very detailed look on the implementation of the bar() function in said descendant (and of course in most cases foo() authors will just assume that bar() descendant will do "something similar to what Base::bar is doing"). In a sense implementation of bar is precondition of the function foo() - which is often not even mentioned in the description of function foo()! Or let's consider even more innocuous class:

class Base {

  virtual void foo(void) {

     ...

  }

  virtual void bar(void) {

    ...

  }

}

This class looks innocent enough, but in reality it's descendants can easily tie foo() and bar() in the unimaginable knots because now you have TWO slots in vtable which play the role of RUNTIME-SWITCHABLE GOTO STATEMENTS.

Compare with interface inheritance. If all the functions are pure virtual then the class which implements these functions is fully autonomous: they can be considered in isolation and surprising jumps from one function to another (or even less surprising lack of jumps) can not longer happen. But if you will then EXTEND this class... then suddenly you back in the square one WRT dreaded RUNTIME-SWITCHABLE GOTO STATEMENT. C++11 solves this problem with it's "final" syntax, but in C++98 there are no mitigation.

Note the first example: if bar() there is pure virtual (and is only ever overloaded once) then you essentially have the same "no-surprises" property even if, formally speaking, you are doing implementation inheritance.

Within Chromium land, M-I is unfortunately used with great abandon - and while it's generally safe with various pure-virtual interfaces, subtle problems like this can crop up. In addition to these inconsistencies, one of the other things that can happen are when the lifetimes of two interfaces are incompatible. Examples include when one interface may be ref-counted, another expected to be owned/deleted, or when weak pointers may get involved (as discussed elsewhere on chromium-dev)

One way to break such a multiple inheritance cycle is to actually have methods used to get implementations of the various interfaces - eg: GetWidgetDelegate(), GetView() - and have internal classes that implement the relevant interfaces and cooperate with the parent class.

Yup. As style guide says: composition is often more appropriate than inheritance. Implementation inheritance is modern version of goto and while we don't ban goto (yes, there are no such rule in style guide) we know that we should use it rarely and sparingly. Yes implementation inheritance (including very dangerous multiple implementation inheritance) is used as if it's the most natural thing to do.

I guess it's because of the unfortunate property of C++ which mixes important and indispensable interface inheritance with dangerous and fragile implementation inheritance, but maybe we can be a liiitle more attentive?

 

Because such solutions require more thought and care to implement, and "in general" M-I doesn't cause problems, it's rarer to see in Chromium code, but in the end can result in clearer code and fewer violations of pre-conditions.

There are no violations of pre-conditions because there are no preconditions. People rarely even think about pre-conditions for virtual methods because it's too hard: it includes precise documentation of all the OTHER methods listed in the class and guidance for ALL descendants. You have the modern incarnation of "spaghetti code", what do you expect to see here? Reliability? Won't happen.

 

That said, Clang at least knows when these M-I issues emerge and when methods may be shadowed / names hidden, so it may be possible to make it warn. However, the algorithm currently used is O(n^2) for the number of classes in the hierarchy, so I'd be a little nervous for compile times.

I'm not all that sure if it's actually worthwhile: if you have couple of classes which have virtual functions with bodies you are on a way to ruin, if you've reached the state where there are multiple inheritance involved then it's time to rethink your design. Clash of the names or no clash of the names.