13
\$\begingroup\$

I have just implemented a quick test of the visitor design pattern in C++ and have a few questions regarding the code.

main.cpp

#include <iostream>

#include "Base.h"
#include "Derived.h"
#include "Visitor.h"

int main() {
    Base base;
    std::cout << "hello\n";
    Base * derived = new Derived();
    Visitor v;
    base.acceptvisitor(v);
    derived->acceptvisitor(v);
}

IVisitable.h

#ifndef IVISITABLE_H
#define IVISITABLE_H

class Visitor;

class IVisitable {
    virtual void acceptvisitor(Visitor v) = 0;
};

#endif

Base.h

#ifndef BASE_H
#define BASE_H

#include <string>

#include "IVisitable.h"

class Base : public IVisitable {
public:
    void acceptvisitor(Visitor v);
};

#endif

Base.cpp

#include "Base.h"

#include "Visitor.h"

void Base::acceptvisitor(Visitor v) {
    v.visit(*this);
}

Derived.h

#include "Base.h"

class Derived : public Base {
public:
    void acceptvisitor(Visitor v);
};

Derived.cpp

#include "Derived.h"

#include "Visitor.h"

void Derived::acceptvisitor(Visitor v) {
    v.visit(*this);
}

Visitor.h

#ifndef VISITOR_H
#define VISITOR_H

class Base;
class Derived;

class Visitor {
public:
    void visit(Base b);
    void visit(Derived d);
};

#endif

Visitor.cpp

#include "Visitor.h"

#include <iostream>

#include "Base.h"
#include "Derived.h"

void Visitor::visit(Base b) {
    std::cout << "Visiting Base\n";
}

void Visitor::visit(Derived d) {
    std::cout << "Visiting Derived\n";
}

And here are some questions, of course I am interested in all remarks you may have not just answers to these.

  1. How do you keep track of includes? Even in this short example I now have includes that are not needed anymore.

  2. Is this #ifndef treatment of header files still normal? I picked it up somewhere but have never actually seen anybody else do it.

  3. Is there any way to treat an object as a superclass without a pointer? I.e. can I do something like this: Base d = Derived();

  4. When can I use a forward declaration like class Visitor; and when do I need to include the actual header file? Or when should I?

  5. Should I make implementations of abstract functions virtual? It seems not to matter.

\$\endgroup\$
9
\$\begingroup\$

Well your code is all broken.
But we will get to that as we answer your questions:

Answers to Questions

How do you keep track of includes? Even in this short example I now have includes that are not needed anymore.

  • Only include a header file if you need the class definition.
  • Forward declare everything else

Using this rule it is easy to only include files you need. In your header files only include other header files if you need them. You only need them if the object you are defining has a member of that type or is derived from that type (or takes a parameter to a method by value).

In all other situations forward declare.
This reduces the header count considerably.

In the source file you include all the header files of object you use to implement your code (that your header files have not included).

#ifndef BASE_H
#define BASE_H

#include <string>  // There is no need for this.
                   // There are no members that use string
                   // You are not deriving from string (who would)
                   // You are not passing a string as a parameter
                   // Remove this.

#include "IVisitable.h"

class Base : public IVisitable {
public:
    void acceptvisitor(Visitor v);  // By the way this is probably wrong.
                                    // You are passing by value and thus will
                                    // Make a copy.
                                    // Here you (if you really
                                    // wanted to pass by value) you should have
                                    // include "Visitor.h".
                                    //
                                    // But you don't want to pass by value you want
                                    // to pass by reference. So you just need to 
                                    // forward declare.
};

#endif

Is this #ifndef treatment of header files still normal? I picked it up somewhere but have never actually seen anybody else do it.

Yes you should always place include guards around header files to protect from multiple inclusion.

Is there any way to treat an object as a superclass without a pointer? I.e. can I do something like this: Base d = Derived();

Well actually what you have written will compile:
Unfortunately it does not do what you expect. Here you have object slicing. The Base part of the object you created is sliced out and copied into d. What you need is a pointer (or a reference).

It looks like you are used to languages like Java. Where all objects are dynamically allocated. C++ has a much superior mechanism that allows us to accurately control the lifetime of the object. The disadvantage is that it adds complexity to the language.

What is called a pointer in C/C++ (to distinguish it from C++ references) would in most other languages be called a reference. C++ has both local objects (automatic storage duration object) and dynamically allocated objects (dynamic storage duration objects) thus we need a convention that allows access to both types of object, hence we use * to refer to objects that are dynamically allocated (to distinguish them from local objects).

BUT it is unusual to use pointers directly in C++ (unless you are implementing some real low level stuff). Most of the time when you dynamically allocate objects you will use a smart pointer that defines the lifespan of the object (much like other languages with garbage collection (but better)). std::shared_ptr<T> would be the equivalent of T in Java.

std::shared_ptr<Base>   d = new Derived();  // dynamically allocated object
                                            // That will be correctly destroyed
                                            // when there are no more references.

When can I use a forward declaration like "class Visitor;" and when do I need to include the actual header file. Or when should I?

As described above.
In the header file (were there is only declaration) only include another header if the header file defines a type that is a member or is used as a parent or used to pass a parameter by value (parameters are infrequent as they are normally passed by reference). In all other cases in a header file you should use forward declaration. In the source file include the header files that define types that you use (ie call methods on).

Note: You only need a forward declaration for objects that are pointers or references.

Should I make implementations of abstract functions virtual? It seems not to matter.

Only virtual methods can be abstract.
If you forget to define a method the compiler will not complain (as you may define it in another compilation unit). The linker will only complain if somebody tries to call the method and can't find a definition. So if you don't call it then there will be no error (but if you don't call it then it does not matter).

When you implement a virtual function in a derived class it is probably best to mark it as virtual to show a subsequent maintainer that it is virtual function (but it is not required by the language).

You should also note that C++11 introduces they keyword override. Which is an indicator that this method overrides a virtual method in a base class. If there is not such method in the base class it is a compilation error.

Other Notes

Your code passes all parameters by value. This is probably not what you want (as a copy will be made). If it is a derived type the base parameter type will be sliced out and passed to the method. So pass by reference.

class IVisitable {
    virtual void acceptvisitor(Visitor& v) = 0;
};                          //       ^^^   pass by reference

Also note that you can have more than one class defined in a file. Thus personally I would have put the IVisitable and Visitor patterns in the same file (they are tightly coupled anyway).

\$\endgroup\$
  • \$\begingroup\$ Wow, another answer. Again I have follow-up questions. :) Above you said if I pass by value I should include the header file but it wasn't necessary, should I have done it anyway? Also what are the different kinds of smart pointers? I have seen at least shared_ptr and auto_ptr. \$\endgroup\$ – Corporal Touchy Jul 14 '12 at 19:38
  • \$\begingroup\$ Did I understand correctly that object slicing is used when passing by copy but not when passing by reference? \$\endgroup\$ – Corporal Touchy Jul 14 '12 at 19:44
  • \$\begingroup\$ If you pass by reference you only need to use a forward declare (so don't include header file). \$\endgroup\$ – Martin York Jul 14 '12 at 20:44
  • \$\begingroup\$ Types of smart pointers: stackoverflow.com/a/8706254/14065 \$\endgroup\$ – Martin York Jul 14 '12 at 20:45
  • 1
    \$\begingroup\$ Slicing is a side affect. Few people use it intentionally. Object slicing happens when you pass a derived object to a base class object and the copy constructor (or assignment) gets invoked (so passing a parameter by value is one of those places). It will not happen when passing by reference or pointer. \$\endgroup\$ – Martin York Jul 14 '12 at 20:47
0
\$\begingroup\$

Your implementation looks mostly good, however I'm not sure if I'd recommend passing your parameters by value. It may be what you're looking for, but generally I'd consider passing by reference or const-reference - it avoids the need for one or more copies, and means you shouldn't get any object slicing as I mention in answer to question 3 below. Unless of course you wanted the copies and/or slicing to occur, in which case your implementation is fine.

Now, to try and answer your questions:

  1. I try to just keep an eye on the #includes and remove any that I don't think are required anymore. If I remove one that was actually required I can always add it back in again. I also tend to make a point of removing #includes whenever I have made changes to a file that I know mean I no longer require a header. It's more art & experience than science though, so I'm not sure I can give much good advice here.

  2. Using #ifndef etc. to protect your include files from being included more than once is definitely still a good idea. Some compilers support #pragma once that does a similar thing, but as this isn't universally supported I'd stick with #ifndef protection.

  3. You could use references for that, which act very similarly to pointers. Allow me to demonstrate:

    Base b = Derived(); // Here b will be an instance of base

    Base& b = Derived(); // In this case b will be an instance of Derived.

    In the first case, the derived that is created will be "sliced" into an instance of Base, meaning that any members that are in Derived but not in Base will be dropped, and b will just be a normal instance of Base. For more information on object slicing, you could read this wikipedia article

    In the second case, you're creating a reference to an instance of Derived. This will act much like a pointer would - you can only directly access members exposed by Base, but the underlying instance is a Derived.

    To answer your comment below, Base& b = Derived() isn't actually valid c++, because the Derived() call creates a temporary, and you can't point a reference at a temporary. The code was just to illustrate a point, rather than as a proper suggestion. What you can do however, is

    Derived d; Base& b = d;

  4. You should include the actual header file for a class whenever the compiler actually needs some information about it's members - for example if you're calling functions on the class, or if you're declaring a value type of the class.

    If you're just passing around pointers or references to a class without actually using it you can usually get away with just a forward declaration.

    My general rule of thumb is to forward declare anything that is needed in a header file, then #include the headers required in the .cpp file. Except when the full declerations are actually required in the header file. This seems to be roughly what you're doing, so I think you've got the right idea.

  5. I'm assuming you're asking whether you should specifically state that void acceptvisitor(Visitor v); in Derived is virtual. This isn't strictly neccesary - as the function is declared as virtual in the IVisitable class then it will be virtual in Derived as well.

    However, you may want to mark it as virtual anyway - this might make it more obvious to anyone reading the code that the function is actually virtual, rather than a normal method.

Hopefully I've been clear enough here, but feel free to leave a comment if you have any further questions or want some clarificaiton.

\$\endgroup\$
  • \$\begingroup\$ The objects have to know about the visitor because the vtable of the objects is used to make sure the right function is called. To make sure, I tried your changes and as expected I got "Visiting Base" twice. \$\endgroup\$ – Corporal Touchy Jul 14 '12 at 14:12
  • \$\begingroup\$ This: Base& b = Derived(); gives me main.cpp:10:29: error: invalid initialization of non-const reference of type ‘Base&’ from an rvalue of type ‘Derived’ -- What am I doing wrong? \$\endgroup\$ – Corporal Touchy Jul 14 '12 at 14:14
  • \$\begingroup\$ @CorporalTouchy Ah, sorry. I obviously didn't think my first suggestion through! I've removed it and added a bit of clarification on the error you're getting. \$\endgroup\$ – obmarg Jul 14 '12 at 14:52
  • \$\begingroup\$ The code is broken, not “mostly good”. See Loki’s answer. \$\endgroup\$ – Konrad Rudolph Jul 14 '12 at 20:30
  • \$\begingroup\$ @KonradRudolph Care to elaborate? I don't see anything actually broken. There's object slicing going on and a bit of excess copying, which I mention. Can't see anything critical mentioned in Loki's answer either, though I've mostly just skimmed it. \$\endgroup\$ – obmarg Jul 14 '12 at 20:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.