3

For a project, I want to hide some implementation details from the library interface. The only way I came up to handle this "efficient" (in terms of coding and maintenance) was with multiple inheritance of abstract and concrete base classes based on the same abstract base. So, yes, I ended up with an extended diamond structure.

I'm usually trying to avoid multiple inheritance whenever possible due to it's complications, so I'm not very experienced in using it and so I'm unsure if the design I'm comming up with has some pitfalls I don't oversee.

Basically I need to provide multiple "tools". Those "tools" have of course some implementation details I want to hide using interface ABCs, for example:

class Tool1Impl : public ITool1;

class Tool2Impl : public ITool2;

However, of course the tools have quite some commonalities.

So I would like to have a common base class, but implementation details again should be hidden through interfaces.

I came up with the following (diamond) scheme :

            ITool                                           
          /   |   \               
        /     |     \
      /       |       \
ITool1  ToolImplBase   ITool2  ... IToolN
  \       /      \      /
   \     /        \    /
    \   /          \  /
   ToolImpl1      ToolImpl2    ... IToolImplN

ToolImplBase implements pure virtual functionality of ITool. ToolImpl1 implements pure virtual functionality declared in ITool1.

Implementation outline looks like:

class ITool {
public:
  virtual int common_foo(int a) = 0;
};

class ITool1 : public virtual ITool {
public:
  virtual int specific_foo(int b) = 0;
};

class ITool2 : public virtual ITool {
public:
  virtual int specific_bar(int c) = 0;
};

class ToolImplBase : public virtual ITool {
public:
  virtual int common_foo(int a) override;
protected:
  int _some_common_data;
};

class ToolImpl1 : public ToolImplBase, public virtual ITool1 {
public:
  virtual int specific_foo(int b) override;
private:
  int _some_specific_data;
};

class ToolImpl2 : public ToolImplBase, public virtual ITool2 {
public:
  virtual int specific_bar(int c) override;
private:
  int _some_specific_data;
};

It seems to do what I want, but I'm not sure if there any potential pitfalls or obvious issues with this. I'm also not sure if I got the "virtuals" in the inheritance scheme all right (not the virtual methods, but the ": public virtual "). One issue is, that MSVC (I'm stuck with 2010 due to some external depencies at this time) gives me a compiler warning C4250:

warning C4250: 'ToolImpl1': inherits'ToolImplBase::ToolImplBase::common_foo' via dominance

Can I just ignore that? I'm not even sure why I'm getting this, because the non-dominant way to "common_foo" is pure virtual, so there actually is no valid "non-dominant" implementation.

Thanks for any input (except for "never use M.I.", I'm trying to avoid it where possible already).

Marco Freudenberger
  • 636
  • 6
  • 13
  • 2
    My small bit: feel free to use MI when required but when you have a diamond then you're probably on the wrong path. Are you sure you're not using inheritance because of outcomes instead of meaning? Two things to consider for ToolImplBase (I can't say more because example is fictional): composition and private inheritance. – Adriano Repetti Nov 17 '16 at 10:28
  • The example isn't that fictional, it's from my actual code, of course simplified. How would you use the composition here? Just delegating all common functionality from ITool in the ToolImplFoo to a contained ToolImplBase? – Marco Freudenberger Nov 17 '16 at 10:32
  • C4250 function `common_foo` is inherited by 2 path, and have different implementation, it's necessary to implement it in final form – Danh Nov 17 '16 at 10:32
  • @Danh: Thanks, but what I understand is that the one path through IToolX is pure virtual, so no real "implementation". – Marco Freudenberger Nov 17 '16 at 10:34
  • It's OK to use virtual inheritance for a pure interface. Some even recommend it. An alternative is to document (as in COM) that only some specific interface, if any, serves as object identity. – Cheers and hth. - Alf Nov 17 '16 at 10:36
  • 1
    *the non-dominant way to "common_foo" is pure virtual* That's exactly what dominance is. A non-pure implementation dominates the pure one. Why MSVC needs to warn about it is anyone's guess. Perhaps its authors dislike or don't understand the dominance rule. Other compilers have no such warning. IMO you can safely ignore this, see [here](http://stackoverflow.com/questions/6864550/c-inheritance-via-dominance-warning), but also see [here](http://stackoverflow.com/questions/7071671/inheritance-by-dominance-is-it-really-bad). – n. m. could be an AI Nov 17 '16 at 10:36
  • @n.m.: thanks for the clarification, that was my thought. – Marco Freudenberger Nov 17 '16 at 10:45
  • 1
    Well, warning has a reason. Dominance may be unintentionally broken. I'd prefer to be warned about that (and to decide to silent the warning or provide an implementation) but well, it's a point of view. Yes, I'd do that if I can avoid diamond. I don't mean it's _better_, just less error-prone (IMO) – Adriano Repetti Nov 17 '16 at 11:46
  • Adriano: I agree to your assessment that it is less error prone. It causes more code thoguh and I've seen code in other projects where delegates were reached through many levels of abstraction before anything was actually executed. In it's extreme I don't find that less error-prone (often causes unintentiional re-implementation of the "same" functioanlity on multiple abstraction layers and hard-to-read code), in my specific case it's thoguht-worthy approach for sure! Thanks for your input ! – Marco Freudenberger Nov 17 '16 at 12:27
  • @AdrianoRepetti "_when you have a diamond then you're probably on the wrong path_" Very strange idea. – curiousguy Nov 30 '16 at 19:20
  • This warning is essentially the stupidest warning, evah. It just states you are using inheritance and overriding as intended. The compiler might as well warn you: "you are using C++; C++ is a subtle programming language, you should probably use a simpler language". – curiousguy Nov 30 '16 at 19:22
  • Not really stupid warning. Diamond (and then dominance) are of course allowed in C++ but more often than not they may be an error (or future cause of errors). It's like a warning for if (a = b) {} – Adriano Repetti Nov 30 '16 at 20:12

1 Answers1

0

There is some explanation about the warning in here: What does C4250 VC++ warning mean?

I think there is no problem with your code because you designed ITool1 as an interface.

If you implement the common_foo function in ITool1, you wouldn't know which implementation of the common_foo will work when you create a ToolImpl1.

You will be careful about the issue but anyone else can legally implement common_foo function in IToolX implementation without an error raising.

My final comment is: why do you need your ITool1 to be derived from ITool? ToolImpl1 is already derived from ToolImplBase so your actual implementation class will always be derived from ITool.

I would change IToolX interface as below

class IToolX{
    public:
    virtual int specific_foo(int b) = 0;
};
Community
  • 1
  • 1
cokceken
  • 2,068
  • 11
  • 22
  • Thanks, I understood the C4250 in general, but not in this spefic case, because it isn't actually implementated in the "Interface" path. -- In regards to your question: I want ITool1 derived from ITool, because, as I said, I want to hide implemnetation details from the user (this is a library). "users" will never ToolImplX, but always IToolX (I perhaps should have mentioned that constructurs are protected and I have a Factory class to do something like: ITool1* createTool1() { return new ToolImpl1(); } – Marco Freudenberger Nov 17 '16 at 10:42
  • If you are developing the project and will always remember your decisions on design there won't be any problem. But when someone else is maintaining the code or you need to add new Tool after a few months there might be a problem. – cokceken Nov 17 '16 at 10:44
  • :what kind of problems would you expect? Isn't it true for all code that there might be a problem if somebody rips it appart because the maintainer doesn't understand the original design? IMHO that is what explicit code comments are for... – Marco Freudenberger Nov 17 '16 at 10:48