Circular include dependencies.

[Gaminic]

Hello,

I'm working on a project and this time around I've decided to split everything up into files, because my previous project ended up being one depressingly large pile of illegible code. Since many of the separate files contain only a few simple functions, I've more often than not defined the entire function inside the header file, removing the need for an attached .cpp file.

The only [non-temporary] .cpp file in my project is the one that includes my main. There's a header file attached to that cpp file that includes all the globally defined variables I need (Yes, yes, I know, "Don't use globals, they eat children", but it was the most logical choice). Every other header file includes these, since (nearly) all functions require these globals to work on.

Now I've got a little problem: one of the globals is a Struct defined in a different header, which... requires the header with the globals. Which is a problem.

I've tried splitting the Struct definition over a header and a cpp file (now the header no longer directly requires the globals), but that broke more than it fixed (to be specific: "a lot" vs "nothing").

Is there any way around this?

[Gaminic]

Additional info:

I could replace the use of globals inside the struct by passing them by reference to its member functions. However, the struct basically keeps a backup of the globals at certain moments during runtime, using a save() and load() function when needed. It's a LOT of globals and having to pass each by reference is going to get very annoying very fast.

[ne555]

¿did you use header guards? http://www.cplusplus.com/forum/articles/10627/
I don't understand your description. ¿could you post some code?

[Disch]

(Yes, yes, I know, "Don't use globals, they eat children", but it was the most logical choice)

It probably wasn't. In fact it's very doubtful. Especially if you're saving/loading them in a struct. I mean the fact that you need to have multiple instances like that (ie: a current state and a saved state) shows pretty clearly that globals were the wrong way to go. After all, globals are "one and only one". If you have more than one, you're contradicting yourself.

A more logical approach would have been to put them all in a struct and then have an instance of that struct for each state.

IE:

// globals - bad
int vara;
int varb;
//...

void save_state()
{
  saved_a = vara;
  saved_b = varb;
  //...
}

void load_state()
{
  vara = saved_a;
  varb = saved_b;
  //...
}

vs.

// struct - better (but not great)
struct State
{
  int vara;
  int varb;
  //...
};

void save_state()
{
  savedstate = currentstate;
}

void load_state()
{
  currentstate = savedstate;
}

But that solution is pretty "blech" also. Really it's hard to say what the best course would be without knowing what the code is doing.

But seriously -- globals are very very very rarely the best option. They only seem tempting to use because they're the "quick and dirty" option... but they end up making things harder and messier in the long run.

The overall solution I really recommend here is to rework this code to remove them.


That said....

I've tried splitting the Struct definition over a header and a cpp file...but that broke more than it fixed (to be specific: "a lot" vs "nothing").

This is probably the best (or rather, easiest) solution to the immediate problem. If done properly this shouldn't break anything. In fact... I actually don't even see how it could break anything.

Could you post the header + struct in question? If they're very large perhaps just post the parts that are interdependent.

[Gaminic]

First off: all my headers have include guards, but that doesn't solve circular dependencies.

For code examples, it would require quite some code before it is clear, so I'll just try to explain it:
Basically, my program reads in a very, VERY large 2D matrix and starts messing around with it, in the meantime saving data to other lists and matrices (who are also very large, but less so because they're generally nxk (k<<n) compared to n²). Every (and I mean every) function accesses data from the matrix and nearly every function accesses and writes to the other lists and matrices. The reason I defined them globally is because:
-efficiency is key. (I imagine "structa.varx[i][j]" is (ever so slightly) heavier than "varx[i][j]".)
-the matrices and lists never go out of scope, until the main exits.
-considering the frequency by which they're used, simply having to type "structa." in front of anything would suck.
-in my previous project [which was very similar], I refused to use globals and passed everything by reference to my functions. This was good at first, until I found myself needing to shuffle things around, and not before long every function required every variable. I did consider passing a struct at that point, but it was quite the hassle to recode and it would still force me to type "structa." in front of everything.

Again, keep in mind that everything this program does relates to the large matrix and, to a lesser extent, the smaller lists.

The struct I'm using now keeps a backup of a few of those smaller lists at regular intervals, so I can reload a state at any given moment. There are at any given moment only two "versions" of those few smaller lists in play: the one that is being worked on and the one saved in an instance of the struct.


Finally, on the inclusion errors:
I'm not sure what went wrong with the split-file. I even resorted to relocating the struct header code to the header that contains the globals, but I kept getting linker errors (LNK2005). I'm guessing I can't have a cpp file defining functions declared in a header with a different name?

[Gaminic]

To provide some code details in regards to the struct, I'll provide the "layout" of the three files in question. main.h contains the globals, myStruct.h contains the struct declaration and myStruct.cpp contains the definition of the myStruct functions:

main.h:

#include "myStruct.h"

myStruct backup;
// Definition of globals 

myStruct.h:

struct myStruct {
  vector<int> vec1, vec2, ...;
  myStruct();
  myStruct(parameters);
  void save();
  void load();
}

myStruct.cpp:

#include "main.h"
#include "myStruct.h"

myStruct::myStruct() {
  // "empty" constructor. Lists are initialized when save() is called.
}
myStruct::myStruct(parameters) { 
  vara = parama;
  varb = paramb;
// etc, simply copying of the lists, but through parameters. 
//This constructor isn't actually used as it's a left-over from the previous project.
// Included it for... I'm not sure.
}
void myStruct::save() {
   vara = globala;
  varb = globalb;
// etc. Copies necessary global lists from main.h
}
void myStruct::load() {
  globala = vara;
  globalb = varb;
// Restores backup to globals
}

[Disch]

-efficiency is key. (I imagine "structa.varx[i][j]" is (ever so slightly) heavier than "varx[i][j]".)

Premature optimization is a fool's errand. Did you actually profile this or are you just making an assumption?

You might be surprised at what code is slow vs what code is fast.

Sacrificing code clarity and expandability for speed can make sense.
Sacrificing code clarity and expandability because you think it might improve speed is ridiculous.


That said I don't really want to bicker any further about whether or not globals should have been used here or not. Personally I think it probably was a mistake, but you seem to feel differently, so whatever.

but I kept getting linker errors (LNK2005)

I don't have error codes memorized. Can you post the full error?

And again it's really hard (read: impossible) to know what you're doing wrong without seeing what you're doing. If you can't post the whole code can you at least post a smaller example that reproduces the problem you're having?

I'm guessing I can't have a cpp file defining functions declared in a header with a different name?

The name of the file has nothing to do with it.

[Gaminic]

I'm not sure what else I could give you. Aside from naming and other includes (that don't include anything themselves, and are guarded), the essence remains the same as the "skeleton code" provided above.

I've recreated the problem by splitting it over files again. This error comes for every variable in the main header file:
1>SolutionContainer.obj : error LNK2005: "struct Solution bestSolution" (?bestSolution@@3USolution@@A) already defined in RunGLS.obj

"Solution" is the struct. It is declared in a header file (guarded, doesn't include anything itself) and defined in a cpp file (includes struct header and main header).

"RunGLS" is the "main" file. The header delcares all the globals and includes SolutionContainer.h. One of the globals is a "Solution" struct, initialized by "Solution bestSolution;". A save() is called early in the main function to actually assign values to the object. The cpp file holds the main() function (which simply calls 3 functions sequentially, which hold all the "real" code but are defined in other files). It includes RunGLS.h and Solution.h (and many other, unrelated headers).

[Disch]

Okay... sorry, I didn't see that post with the code when I made my post. Looks like it slipped in before I posted.

Anyway I'm at work now so I won't be able to give a look until I get some free time =(. But maybe someone else will be able to help before then.

[ne555]

SolutionContainer.obj: error: "struct Solution bestSolution" already defined in RunGLS.obj
I guess that it is because of

//main.h
myStruct backup;
// Definition of globals 

Every cpp that include that header is having an object called backup. The linking gets confused of wich 'backup' are you refering to.

If you want that every source has its own independent object use static myStruct backup;
If you want to only 1 object to exists and they all share that object use extern myStruct backup; (you will have to define it in some source)

[Gaminic]

Isn't the entire point of defining something globally that every file has shared use of the variable/object? I'm confused that it would work differently for objects compared to "regular" variables...

[Disch]

I'm confused that it would work differently for objects compared to "regular" variables...

It doesn't. You can't do that with regular variables either.

The thing to remember is that each cpp file is compiled independently. After all of them are compiled, they are then linked together.

This issue arises because each cpp file tries to make it own variable (since they are unaware that any other cpp files are making the variable). As a result, the linker gets confused because you have multiple variables with the same name and it doesn't know which one to use.

The solution is to make the variable extern in the header, then actually define it (without extern) in a single source file. Using extern is basically the same concept as having a function prototype. It tells cpp files that there is a variable with that name, but they don't actually create the variable. When you don't use extern, it's like giving the variable a "body", kind of like defining a function body.

[Gaminic]

Well, then I'm doubly confused. My globals from the main header are used everywhere and the program runs without any odd behaviour. As far as I can tell, all separate files use the same global variables without problem. The problems only started once I introduced a self-made class into the mix. Was this just my compiler being poka yoke, or am I to expect problems in the future by miss-using globals?

[edit]
Additionally, shouldn't the header guards block this "each file tries to make its own variable"?
[edit2]
Before I forget: thanks for the explanation! Even more so because it's in terminology I understand.

[Nisheeth]

Additionally, shouldn't the header guards block this "each file tries to make its own variable"?

Header Guards guard only the header files.
They make sure that anything defined in the header is not compiled multiple times. But, This doesn't apply to the corresponding cpp files.

[Gaminic]

Yes, but a) Aside from my main cpp file [and the cpp file for the struct mentioned above, which by now has been deleted], everything is written in header files. Because they're so small (2 large ~ 10 small functions), I saw little point in splitting the code over header and cpp files.

Also, b) All globals (the point of this topic) are declared in a single header file (the "main" header), which (as always) is guarded.

Unrelated c) Using the "extern" is a bit of a hassle, isn't it? I'd have to declare them as "extern" in the main header and then "redefine" them in a cpp file. To be honest, my current solution (defining the class inside the main header and keeping all globals there, without 'extern') is much easier and since there's a good chance there won't be any new classes or globals in the program, I think the downside of such "bad practice" is negligible [for this project]...

[Disch]

My globals from the main header are used everywhere and the program runs without any odd behaviour.

I've had this work on really old compilers (VS4 mostly -- back when I was first starting out and was doing the whole global thing like you are). More recent compilers shouldn't let you get away with it, though. What compiler are you using?

Was this just my compiler being poka yoke, or am I to expect problems in the future by miss-using globals?

Expect problems in the future.

Also, b) All globals (the point of this topic) are declared in a single header file (the "main" header), which (as always) is guarded.

Guarding your header doesn't do anything for this particular problem. As Nisheeth said, it just prevents a header from being included more than once in the same cpp file.

Look at it this way. Imagine you have two separate cpp files with two separate globals who just happen to have the same name:

// a.cpp

int myglobal = 0;

void a_func()
{
  // do something with myglobal
}

// b.cpp

int myglobal = 1;

void b_func()
{
  // do something with myglobal
}

From this code, it seems like the programmer's intent was to have two unique variables. The fact that they share the same name is merely a coincidence. a's global and b's global appear to be two separate variables.

What you have to remember with #include is that it's basically just a copy/paste operation. All the compiler does when it comes across an #include line is start compiling the header file as if it were copy/pasted into the cpp file. The end result is that putting a global in a header looks exactly the same to the compiler as the above code snippit.

Let's replace the above with a simple header:

// header.h
int myglobal;

//a.cpp

#include "header.h"  // the compiler will simply replace this line with the header's contents
   // effectively declaring int myglobal; right here

//... 

//b.cpp

#include "header.h"  // same thing

//... 

So as you can see, it's just as confusing to the compiler and linker. They see multiple cpp files wanting to create what appears to be individual variables which just happen to share the same name and they freak out and complain.

c) Using the "extern" is a bit of a hassle, isn't it?

I suppose. But use of shared globals is such a rarity that it practically never comes up (or rather, it should be such a rarity).

I'd have to declare them as "extern" in the main header and then "redefine" them in a cpp file.

Yup. There's a shortcut you can use with macros though. Since you seem to be committed to sticking with globals...

// globals.h -- or whatever header has your globals
#ifndef GLOBAL
#define GLOBAL extern
#endif

GLOBAL int myglobal;
GLOBAL int whatever;
//...  etc 

// globals.cpp -- a single cpp file that does nothing but instantiate your globals
#define GLOBAL
#include "globals.h"

// no need to put anything else in this cpp file 

// all other cpp files
#include "globals.h" // <- they can just #include normally 

How this works is it hides the extern keyword behind a GLOBAL macro. globals.cpp manually #defines GLOBAL as nothing, which prevents the header from #defining it as extern. The end result is that extern is put there automatically for all files except for globals.cpp. Resulting in the desired effect of having them instantiated once and only once.

[Gaminic]

So if I understand correctly, the problem has always been there, but it's just been hidden since I'm only using a single cpp file and dumped the rest into header files, which don't get compiled, thus from the compiler's perspective all code might as well be in that single cpp file?

Well, guess I've learned my lesson in regards to globals. I'm going to stick with it for this project (it's nearing completion, code-wise). The only-headers method has sort of done what it's supposed to do (all I really wanted was to keep my code organized. In my last project, everything was one big cpp file through which I tried to navigate by making extensive use of the Collapse All hotkey).

Now that you're quite familiar with my project (and there are many similar ones to follow), what do you recommend? Replace all globals with a struct and pass it along by reference? Or is there a more awesome way?


P.S.: Some "side-questions" I hadn't answered until now to prevent sidetracking too quickly:
a) I'm using MVC++2010 (Express, or sometimes Ultimate, when I have access to it).

b) (In regards to "Efficiency is key -> no structs", Profiling or Assumption?) My first project (again, very similar)'s first version was class based. I've done OOP programming before. It's structured and looks pretty. However, I felt there was a lot of overhead. When I started reading implementation techniques for the problem at hand, I saw (nearly) none of the authors used classes. I remade that first project (using a single cpp file, with "globals") and noticed it was much faster. There were no other major differences between the two versions, so I concluded classes were slow. I might have simply been using them inefficiently.

c) There were only two side-questions but I felt sad when I noticed I started a numbered list with only two points so I'm just going to fill this up a bit.