Try including game.h in player.c.

It is important to understand what the problem here is.

The actual problem is this. game.h includes player.h, and player.h includes game.h.

So, when the compiler enters game.h, the compiler will hit the #include and go to player.h. The compiler will then hit the #include "game.h" in player.h. However, since game.h has its include guards up, it will immediately exit, without having included the vital symbols into player.h.

Adding a forward declaration of the class game solved the problem because the class player never got to see the declaration of game.

Important Object Lesson: never include anything in a header file. Especially if that header can cause a circular reference. Always rely on forward declarations, and use Pimpl to hide uses of objects that forward declarations work with.

what is wrong with including headers ?

just do it right, instead of trying to be clever.

Can you give anti-cookies for retarded statements like this?

It may be that the lesson was too strict.

You can include a header if you are certain that it will not subsequently include this file. So obviously standard library files are all perfectly fine to include.

The problem comes from willy-nilly including headers just to get a symbol defined. This is the quickest way to back oneself into such a corner. And forward declarations won't buy your way out of every problem.

It should also be pointed out that including headers inside headers increases compile times. You should try to write headers such that they do not include features that are important only for the implementation of the functions or classes. So, if a class uses a std::vector internally, which you cannot forward declare due to being templated and almost certainly not being used by reference, you should hide that implementation detail inside another class that you can forward declare.

So, rather than:

#include <vector>

class MyClass
{
public:

private:
  std::vector<SomeType> m_anArray;
};

You have:

struct MyClassData;

class MyClass
{
public:

private:
  MyClassData *m_pData;
};

You define MyClassData in your .cpp file. You allocate m_pData in the constructor, and delete it in the destructor.

As someone who has worked on large projects with over a thousand files, both on projects that operate like this and those that don't, trust me; you will be thankful. Especially for things like std::vector and std::map, which typically have gigantic implementations.

In your case, you should #include<stdio.h> in your header, but you should also make sure that no other header needs to include this specific header file.

My apologies; I thought I was being clear. The problem presented in this thread happened because of including headers inside of other headers, rather than using forward declarations. Had edfredcoder been writing his code with respect towards not including header files inside header files, then this would have been averted.

If you are deriving a class from another class, then that means that you intend for them to do so. If you do not intend for them to do so, locking the actual data members up in a struct that they do not have access to is a pretty good way of saying, "Don't derive from this." This is even more true in light of the fact that C++ has no concept of "final" the way that Java does.

If you do intend to allow derivation, then all you need to do is this:

Note that, in BaseClass.cpp, we do not define BaseClassImpl. Instead, it lives in its own header, where it includes all manor of things:

Even though we have put BaseClassImpl in its own pair of files, you still keep compile times down. This is because the only files that actually need to include BaseClassImpl are .cpp files that actually derive from BaseClass. The mere use of BaseClass, which is 90% of the time you need to include it, will not include the junk that BaseClass needs in order to function.

So deriving a new class from BaseClass requires deriving a new BaseClassImpl too. Notice that BaseClass has a protected constructor that can take a BaseClassImpl object to use directly. This would be a class that your derived class constructor uses, when it creates the object.

You do not need to derive a new BaseClassImpl; you could simply let the base class have its Impl data, and your derived class would have Impl data for your own needs. The fact that the data pointer is protected allows you to access it from your derived class.

If you only ever use a type by pointer or by reference, then you should use the short declaration (class game;). If you use actual objects of the type in the file, you need to include the header.
Example:
-- foo.h --

#ifndef FOO_H
#define FOO_H

class Bar; // no need to #include, because we only use pointers/references

class Foo {
  public:
    void doSomeStuff(const Bar& bar);
};

#endif

-- bar.h --

#ifndef BAR_H
#define BAR_H

#include "foo.h"

class Bar {
  protected:
    Foo myFoo;
};

#endif

There are many times when this does not work. You cannot forward declare template classes for example. So any STL class, or many of Boost's classes, cannot be used this way.

There is a problem with this logic: it only becomes a problem when a project has a number of code files well into the hundreds.

The process of converting from non-Pimpl to Pimpl coding style is not an automatable task. So you cannot write a simple script to read through files and convert them. You are in effect saying that at some point, you expect all development to stop and everyone to go through and perform major refactoring on virtually every system.

This is simply not reasonable. I've been on such projects, and it would have been virtually impossible to do this on those files. At the very least, it would have taken a long time.

The reason that performance optimizations come last is because they are almost always localized fixes. You change how a small set of functions or classes work. Converting a codebase to Pimpl style requires converting the entire thing to get the benefits of it.

If I may ask, why?

C++ has proven time and again that if you give someone the opportunity to do the wrong thing, then they will do so and do it often. If it is wrong to derive from a class, if doing so will never serve a valuable purpose, then locking the class off from derivation is perfectly reasonable.

If the "not-base" class does not have any actual virtual methods on it, deriving from it is of little value. You cannot change how code that expects the "not-base" class will behave just from deriving. This can only happen with virtual methods, and virtual methods are defined by the creator of the library, not the user.

So I do not know what it is that you expect to get out of being able to derive classes that are not built for derivation.

Unread comments are of no value. Counting on programmers to read your comments is, to put it mildly, unwise. Indeed, it might be wiser to count on programmers not reading your comments.

The set of code that will not be used incorrectly is the set of code that cannot be used incorrectly. This is one of the reasons why exceptions are preferred over error codes; they cannot be ignored. Thus, while the program may crash from the uncaught exception, it will crash exactly where the error happened, and not miles away with some arbitrary corrupted data.

Self-documenting code is the most effective way of keeping down user error. It would not be unreasonable to say that it is the only way to do so.

While I understand your point, I just don't find it compelling.

Take anti-lock breaks (ABL) on cars for example. ABL doesn't do anything that the driver couldn't do. The driver can recognize that the car is losing traction and pump the breaks. Most drivers learned to do this at some point.

This does not mean that ABL is a bad idea. It's automatic; no thought is required. It turns on when needed. And it is more sensitive to traction loss than any human can possibly be, and it can pump the breaks faster than a human foot can move.

The fact that a programmer can write their code around the minefield that is your library doesn't mean that they should have to. Part of good API design is making it impossible for the person to do the wrong thing. It seems poor service to the end-user to force them to check every function/object's use in the manual/code comments to see if it violates some requirement. Certainly, when a programmer is up at 6AM trying to resolve some issue for a build, it would be best that all stupid mistakes were covered.

And of course, this assumes that the comment is well-written. I have seen several warning comments that might or might not apply to my particular use, depending on interpretation.

In fact, I was just writing some matrix functions today when I came upon the following conundrum. Matrix inversion can be impossible for certain matrices. The test for this is fairly straightforward: if the determinant is zero, then the matrix is not invertible. Now tell me, which of the following is the best way to handle this?

Matrix4x4 someMat;
if(someMat.Determinant())
{
  Matrix4x4 inverse = Inverse(someMat);
  //do stuff with inverse.
}

or

Matrix4x4 someMat;
if(someMat.IsInvertable())
{
  Matrix4x4 inverse = Inverse(someMat);
  //do stuff with inverse.
}

or

Matrix4x4 someMat;
boost::optional<Matrix4x4> inverse = Inverse(someMat);
if(inverse)
{
  Matrix4x4 theInv = inverse.get();
  //do stuff with theInv.
}

Code segment 1 is obtuse. Without significant knowledge of matrix math, which a surprising number of people using matrices lack, it makes no sense to check the determinant of the matrix before inverting it. Since it seems like a strange operation, users will likely decide that it is something special about this particular usage, and not do it in their code.

Code segment 2 is better, since there is an explicit function for inverse detection. The name of this function suggests its purpose. However it does not make you test your matrix for inversion before inverting it. It should also be pointed out that the inverse function itself will compute the determinant as part of its computation, so it happens twice needlessly.

Code segment 3 is, to my mind, the best. The return value is optionally returned. This forces the user to check that there is a value before using it. The optional::get function will not do nice things if it doesn't actually contain a value. In short, you can't possibly screw up, even if you're awake at 6AM, trying to get a build out to the publisher before a deadline. It is also immediately obvious to anyone using the function that it can fail, which helps reinforce the notion that it can fail.

I think I prefer...

Matrix::Matrix4x4 oMatrix4x4;
Matrix::Matrix4x4 oInverseMatrix4x4;

if(oMatrix4x4.determinant() != 0)
{
    oInverseMatrix4x4 = oMatrix4x4.inverse();
    std::cout << "The matrix was inverted." << std::endl;
    // etc...
}
else
{
    std::cout << "The matrix cannot be inverted because the determinant is zero." << std::endl;
    // etc...
}

// etc...

Or...

Matrix::Matrix4x4 oMatrix4x4;
Matrix::Matrix4x4 oInverseMatrix4x4;

try
{
    oInverseMatrix4x4 = oMatrix4x4.inverse();
    std::cout << "The matrix was inverted." << std::endl;
    // etc...
}
catch(Matrix::DeterminantIsZeroException ex)
{
    std::cout << "The matrix cannot be inverted because the determinant is zero." << std::endl;
    // etc...
}

// etc...

_{Note: this is just an example. I haven't done enough exception handling [in C/C++] to know if this is valid code.}

I've heard that try...catch can be extremely expensive, however, so that may be something to consider.

I seem to recall you yourself mentioning something about not optimizing too soon?

Pimpl is a fairly common C/C++ programming idiom; using it is not "destroying C++". And when touching a certain header file that is commonly included can cause a 1 hour recompile of a project (I have born witness to this), the "sake of compile time" clearly becomes an issue of significant importance.

Tell that to KDE people