4

I used to write interface classes like this:

class SomeInterface
{
public:
    virtual void doThings() = 0;
    virtual void run() = 0;
};

class OtherInterface
{
public:
    virtual void doStuff() = 0;
    virtual void run() = 0;
};

Which meant that I could easily inherit several interfaces and use it like this

class ConcreteClass : public SomeInterface, public OtherInterface
{
public:
    virtual void doThings() { std::cout << "do things" << std::endl; }
    virtual void doStuff() { std::cout << "do stuff" << std::endl; }
    virtual void run() { std::cout << "do run" << std::endl; }
};

int main()
{
    ConcreteClass myObject;
    myObject.run();
    return 0;
}

But I read recently about the Non-Virtual Interface idiom (stuff like this http://www.gotw.ca/publications/mill18.htm), and following those guidelines I should instead write my interfaces like this:

class SomeInterface
{
public:
    void doThings() { doThingsImplementation(); }
    void run() { runImplementation(); }
private:
    virtual void doThingsImplementation() = 0;
    virtual void runImplementation() = 0;
};

class OtherInterface
{
public:
    void doStuff() { doStuffImplementation(); }
    void run() { runImplementation(); }
private:
    virtual void doStuffImplementation() = 0;
    virtual void runImplementation() = 0;
};

Which means the rest of the code now looks like this

class ConcreteClass : public SomeInterface, public OtherInterface
{
private:
    virtual void doThingsImplementation() { cout << "do things" << endl; }
    virtual void doStuffImplementation() { cout << "do stuff" << endl; }
    virtual void runImplementation() { cout << "run" << endl; }
};

int main()
{
    ConcreteClass myObject;
    myObject.run();
    return 0;
}

...except that now it doesn't compile because the call to run() has become ambiguous (error: request for member ‘run’ is ambiguous). What used to be a harmless overlap in the interfaces now results in name collision.

Rereading the argument for the Non-Virtual Interface idiom, I get the feeling that the issue is because I'm applying it to virtual functions that are pure virtual functions. Is that it? Should I only use the NVI idiom for non-pure virtual functions?

Edit 1: Someone remarked that the run() function could be in its own interface class. That actually makes a lot of sense: either both run() have the same meaning and the interface should be factored, or they have different meaning and we want the compiler to say things are ambiguous. This still leaves us with a diamond inheritance issue when using NVI that isn't there with pure virtual functions

Edit 2: It turns out that, if I factor out run() as a pure virtual function in its own interface class from which both SomeInterface and OtherInterface inherit, things don't actually behave as well as I thought. If I introduce CombinedInterface (inheriting from both interface classes) and have ConcreteClass inherit from that, I can call run() on myObject but I cannot use it polymorphically from a CombinedInterface* (again, the call is considered ambiguous).

Conclusion: My takeaway here is that 1) Interfaces should either not overlap, or be factored out. and 2) Inheritance between interface classes should always be virtual

Eternal
  • 2,648
  • 2
  • 15
  • 21
  • What diamond inheritance? I'm not sure what the issue is. – Quentin Oct 22 '20 at 16:10
  • 1
    @Quentin asked myself the same. The `class ConcreteClass : public SomeInterface, public OtherInterface` is missing from the second version of the code, I suppose because it is the same as in the first – 463035818_is_not_an_ai Oct 22 '20 at 16:12
  • @cigien It's actually not diamond inheritance per se, it's a name collision between the runImplementation methods. Virtual inheritance probably won't help here. – Anonymous1847 Oct 22 '20 at 16:13
  • @Anonymous1847 Aah, that's possible. I didn't really read the question thoroughly, I just picked up on a couple of the terms, and suggested a link that might help :) – cigien Oct 22 '20 at 16:15
  • What's the c++14 tag for? If that just *happens* to be the standard you're compiling with, then you can remove that tag. – cigien Oct 22 '20 at 16:15
  • your question would be more clear if instead of speculating you post an actual [mcve] and the compilers error message. – 463035818_is_not_an_ai Oct 22 '20 at 16:19
  • It worked on my machine. I'm not sure what problem you are running into. A [mcve] would be helpful to see what you've done differently than what I've done. – Eljay Oct 22 '20 at 16:22
  • @Quentin actually no, the second version doesn't make sense when `ConcreteClass` is the same as in the first – 463035818_is_not_an_ai Oct 22 '20 at 16:24
  • 1
    @cigien -- virtual inheritance is **not** a solution to a coding problem. The decision to make a base class virtual comes from the design of the class hierarchy -- there are situations where having two copies of the same base class is appropriate, and (much more common) situations where it is not. If implementing that design produces name conflicts, change the names. – Pete Becker Oct 22 '20 at 17:27
  • @PeteBecker Sure. I commented without considering what the issue at hand actually was. I've deleted the comment, thanks :) – cigien Oct 22 '20 at 17:53
  • @cigien — I’m leaving my comment in place, because it addresses an important misconception, even though it now has no antecedent. – Pete Becker Oct 22 '20 at 19:38
  • @PeteBecker Oh, absolutely, it's a helpful comment :) – cigien Oct 22 '20 at 19:55
  • It seems that the issue isn't as obvious as I thought. I've edited my question to make it more explicit – Eternal Oct 23 '20 at 07:52
  • A thing I noted is that while _you_ may have come across that article recently, the article itself is not recent at all. Many things have changed in the intervening 19 years since that was published and the world - and C++ itself - is now significantly different and more evolved. I would take any guidelines that old, and many versions of C++ out of date, with a [large] grain of salt. – Tanveer Badar Oct 23 '20 at 08:00
  • @TanveerBadar Well, that is true, but as far as I can tell all the arguments in favor of NVI still hold, and the issue I'm raising was already true back then – Eternal Oct 23 '20 at 08:10
  • That's certainly correct. I am merely saying there might be a better way to achieve what you are doing now than back then. As for such a way, I think if you moved your `run()` into a base "interface" class that may get rid of the ambiguity. I think you'll need to still inherit virtually from that since the name will still be ambiguous otherwise. – Tanveer Badar Oct 23 '20 at 08:20

2 Answers2

2

You just need to shadow the conflicting name in the derived class.

class ConcreteClass : public SomeInterface, public OtherInterface
{
  public:
    void run() { runImplementation(); }
  // rest of the class

Doesn't shadowing defeat the purpose of NVI?

Oh, but what is the purpose of NVI? It's not there for its own sake, we usually add pre- and post-operations in the non-virtual part as well. Now if the pre- and post-operations are different in SomeInterface and OtherInterface, then you have choice but combine them somehow in the derived class.

class ConcreteClass : public SomeInterface, public OtherInterface
{
  public:
   void run() {
     SomeInterface::preRun();
     OtherInterface::preRun();
     runImplementation() // etc      
 }

But if they are the same to begin with, then perhaps run should be in its own interface, inherited virtually by both SomeInterface and OtherInterface. (Virtually because otherwise there are still two copies of run, each one in its own base subobject).

n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243
  • Shadowing kinda seems to defeat the purpose of NVI, but your second point makes a lot of sense – Eternal Oct 23 '20 at 08:35
  • @Eternal added more info to the answer – n. m. could be an AI Oct 23 '20 at 10:38
  • I think there was an editing issue, because there's a "This" alone by itself – Eternal Oct 23 '20 at 11:18
  • @Eternal indeed, removed. – n. m. could be an AI Oct 23 '20 at 11:42
  • As far as I understand, the point of NVI is 1) to keep the virtuality as an implementation detail, not part of the interface, and 2) to give enough control to the base class that it can later add pre- and post-operations if needed. Using your solution means that when writing the base class I should anticipate that people may shadow the function and provide pre- and post-operation functions, hoping they won't forget to call them... that seems unsustainable. Also, don't you get 3 distinct behaviors depending when calling run() from a pointer to the derived class or to each base class? – Eternal Oct 23 '20 at 12:35
  • I made an edit to the question, in which I factored the `run()` function into its own interface as you suggested. While that makes the design better, that doesn't change the core of the issue – Eternal Oct 23 '20 at 12:45
  • @Eternal That's because it needs to be inherited virtually, forgot to mention that. Editing,.. – n. m. could be an AI Oct 23 '20 at 13:08
0

You need to specify which interface's run you want to call, because in general they do different things.

int main()
{
    ConcreteClass myObject;
    myObject.SomeInterface::run();
    // or
    myObject.OtherInterface::run();
    return 0;
}
Caleth
  • 52,200
  • 2
  • 44
  • 75
  • SomeInterface::run() is pure virtual. – n. m. could be an AI Oct 23 '20 at 10:38
  • @n.'pronouns'm. This disambiguates the `SomeInterface::runImplementation` version – Caleth Oct 23 '20 at 11:30
  • Sorry looked at a wrong version of the code... both SomeInterface::run() and OtherInterface::run() call runImplementation(), but there's only one runImplementation() so you end up calling it twice. – n. m. could be an AI Oct 23 '20 at 11:41
  • @n.'pronouns'm. I agree that in some circumstances your answer is what is wanted. I guess it's good that OP has a hard error here, as there are two valid cases that need to be disambiguated – Caleth Oct 23 '20 at 12:06