22

I'm one of those people that has to get their code to compile with 0 warnings. Normally I respect the compiler and if it issues me a warning I take it as a sign that I should touch up my code a little. If I have to tell a compiler to ignore a given warning, I twitch a little.

But this one I can't seem to get around, and from what I can tell I haven't done anything "bad". Does anyone think that this is a poor design? I can't see anything particularly nasty about it (except for the "evil diamond") but it's perfectly valid and useful code. But it generates (in MSVC) a level 2 warning!

class IFoo
{
public:
    virtual void foo() = 0;
};

class Bar : public virtual IFoo
{
public:
    virtual void foo() { std::cout << "Hello, world!"; }
};

class Baz : public virtual IFoo
{

};

class Quux : public Bar, public Baz
{

};

Now if I create a Quux object it should be expected to call the Bar::foo implementation. MSVC is very helpful: it warns me for not being ambiguous enough?

warning C4250: 'Quux' : inherits 'Bar::Bar::foo' via dominance

Now I recognize I can turn this warning off with a pragma, but that's not the question I'm trying to ask here. Is there a reason I should be listening to the compiler here, or is this just an extremely overzealous warning?

Shirik
  • 3,631
  • 1
  • 23
  • 27
  • 1
    Honestly, using virtual inheritance at all is bad design in my opinion. It's just too complicated for very little gain. I would *seriously* think twice (or three or four times) before using it. – Peter Alexander Aug 15 '11 at 22:50
  • 1
    The code might not be bad, but it is fragile. Relying on obscure disambiguation rules to select the function to call doesn't make the code very easy to read or maintain. – Bo Persson Aug 16 '11 at 08:15
  • 4
    @PeterAlexander "_It's just too complicated_" complicated how? – curiousguy Nov 02 '11 at 22:30

3 Answers3

12

When performing virtual inheritance, it is a bad idea to not explicitly override every member in the most derived class. Else, you are asking for your code to die a horrible death when someone changes one of your base classes that inherits from the virtual base. There's nothing actively wrong with this, your program won't crash or anysuch, but it's a maintainability bad idea. If you want to call the Bar::foo version, then you should just delegate to it in Quux::foo.

Puppy
  • 144,682
  • 38
  • 256
  • 465
  • I think this is a good point. It doesn't quite fit in the actual problem for me to move everything to the most-derived class (the above is, of course, a simplified example) so I may have to double-check my overall design. (I do expect users of the Bar class.) – Shirik Aug 15 '11 at 23:00
  • 3
    Nonsense. Just because you don't practice something doesn't make it unsound/bad/whatever. Just say: "I am not used to that." – curiousguy Nov 02 '11 at 22:33
  • 1
    @curiousguy: I never said that I don't practice it, and I gave a perfectly good reason why it's bad. If you want to suggest that I'm wrong, then give an objective reason why it's not bad. – Puppy Nov 02 '11 at 23:37
  • 3
    "_die a horrible death_" horrible death = compiler message whenever you introduce an ambiguity. Horrible, indeed. (lol) – curiousguy Aug 07 '12 at 04:48
  • 1
    Actually, what happens with virtual function table in this case? Is there 1 entry for foo() or 2? If there is only one entry and foo() is overridden both in Bar and in Baz, what would that do to a virtual funtction table of a final class? – Serge Oct 06 '15 at 00:08
  • "it is a bad idea to not explicitly override every member in the most derived class" You can't always do that, especially when writing template code. The dominance rule is here for a reason. "you are asking for your code to die a horrible death when someone changes one of your base classes" regardless of whether virtual inheritance or dominance are in use, no big surprise here. – n. m. could be an AI Feb 18 '17 at 21:17
2

As far as the runability of your code is concerned, it is just there to remind you that Bar is the dominant implementation of foo. It is just there to inform you, it's not really a warning, so that if you're debugging and think it's Baz you don't pull your hair out :).

djhaskin987
  • 9,741
  • 4
  • 50
  • 86
  • 1
    Personally, at least, I view level 4 warnings as "just to inform you." Level 2 (which this is) seems to be more along the lines of "what are you doing here...". (For comparison: C4390 is level 3, and that's usually bad.) – Shirik Aug 15 '11 at 22:58
  • 3
    I would be interested in a compiler that informs me of every non-obvious C++ rule used to compile a program. For every overloaded function call, there would be like 5 messages; for every builtin operator called by overloading resolution; there would be 15 messages; for every template overloaded function call, there would be 30 messages. The compile log would be a lot bigger than the program source code. – curiousguy Nov 23 '11 at 05:19
1

Is there a reason you aren't writing:

class Quux : public Bar, public Baz
{
    using Bar::foo;
};

?

This gives you the same level of reuse, without the fragility.

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720